Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1613 #1788

Closed
wants to merge 5 commits into from
Closed

Fix #1613 #1788

wants to merge 5 commits into from

Conversation

shad-k
Copy link

@shad-k shad-k commented Aug 18, 2019

Summary

Adds a init hook to the plugin lifecycle.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

#1613

@shad-k
Copy link
Author

shad-k commented Aug 18, 2019

@Swader have added the init hook. Would you be able to test it and let me know if this works for you?

@Swader
Copy link

Swader commented Aug 19, 2019

Yup, thanks, testing it today!

@flozero
Copy link
Collaborator

flozero commented Aug 19, 2019

Hello thx for the pr. @Swader Can you provide a real example of your need please it could be great and easier to understand your need maybe it can be relevant for other people.

@Swader
Copy link

Swader commented Aug 19, 2019

@f3ltron it's described here: #1613

I'm currently trying to figure out how to test this on an existing project..

@Swader
Copy link

Swader commented Aug 19, 2019

TL;DR: I'm building a doc generator so I need a script to prepare the MD content dynamically BEFORE the rendering process, so that the rendering happens on generated materials, rather than before they're prepared. I could write a separate npm script for that, but I wanted this to be part of Vuepress to integrate somewhat properly.

@kefranabg
Copy link
Collaborator

@Swader Sorry but I'm still not sure to understand 😅Is this the kind of feature you're looking for ?

@flozero
Copy link
Collaborator

flozero commented Aug 19, 2019

hum look at my plugin https://github.com/f3ltron/vuepress-plugin-docgen. I did the same thing. But i face an issue on it about the dev process. I need vuepress re render every markdown i create after changes.

Is that your need at the end ?

@Swader
Copy link

Swader commented Aug 19, 2019

@f3ltron yes I think so.
@kefranabg see response in original issue, should be clearer?

@flozero
Copy link
Collaborator

flozero commented Aug 19, 2019

Ok swader i am working on the vuepress eco system maybe i could help you on it as soon as possible. I am not sure that the hook will answer 100% of your need so if you need a reload at any change you make.

@Swader
Copy link

Swader commented Aug 19, 2019

All I need is for the init hook to run fully and syncrhonously before the ready hook, so that I can prepare ALL the documents for generation before the generation begins. This PR seems to solve it, but I can't figure out how to test this on an existing project. @f3ltron can you help with that please?

@flozero
Copy link
Collaborator

flozero commented Aug 19, 2019

Are you familar with the yarn link etc ?

@Swader
Copy link

Swader commented Aug 19, 2019

I am not, no. I have the fork cloned into a local folder due to #1790 but I have no idea how to temporarily make that clone active instead of the global vuepress I currently have installed.

@flozero
Copy link
Collaborator

flozero commented Aug 19, 2019

What you can do for now to test is.

git clone his repo on his branch

git clone -b feature/issue-1613 https://github.com/shad-k/vuepress.git

make sure you clean you vuepress dependencies on you project delete it on package.json if you have. Delete package lock / node_modules.
Install it again without vuepress this time

cd vuepress 

it will install and run yarn tsc 
yarn bootstrap

// it will link vuepress
yarn register-vuepress

now you can link you local vuepress to your project

go to your project.

npm link vuepress 

You should not have vuepress in you package but present in your node_modules

from here you should be able to add this in your config vuepress files

async init() {
    // ...
  }

I didn't try it myself but you could use it here. I am not sure it will solve your problem but tell me

@Swader
Copy link

Swader commented Aug 19, 2019

I currently have vuepress installed globally, will this not interfere with the local install?

@flozero
Copy link
Collaborator

flozero commented Aug 19, 2019

if you work with link packages its better to delete if globally. It's better to never use global packages

@Swader
Copy link

Swader commented Aug 20, 2019

@f3ltron that seems like a strange thing to say given that official instructions recommend adding it globally by default. But I'll try the remove thing, sure, thanks!

@flozero
Copy link
Collaborator

flozero commented Aug 20, 2019

It should be a mistake. Let me know if you have problems

@Swader
Copy link

Swader commented Aug 20, 2019

@f3ltron thank you, process works perfectly. I seem to have run into some other bug that causes an infinite loop of some kind, though, so debugging that now, but almost 100% sure it's because of my project, not this fix. Looking into it, hopefully will have this resolved tonight and @shad-k can get paid :)

@flozero
Copy link
Collaborator

flozero commented Aug 20, 2019

Super swader dont hesitate to re open issue and mention me. If it's all cool for you let's close this one.

@Swader
Copy link

Swader commented Aug 20, 2019

Let's not close it, I would like to see this PR merged. Is that not an option? What's the blocker, if it works as intended?

@flozero
Copy link
Collaborator

flozero commented Aug 20, 2019

When you have done your test let us have a repo to reproduce your need that we can check if it's interesting to implement it for others

@Swader
Copy link

Swader commented Aug 20, 2019

I think there's a language barrier of some kind.

My repo is at https://github.com/status-im/nimbus-docs-suite

My goal is to run preparation steps before ready hook. This init hooks helps with that. I have tested this and it works, but there's a different break now because Vue uses some kind of progress bars for server/client build output and this results in a rendering loop - I guess I have to push my plugin's console logs into those progress bar statuses somehow, so that's what I have to figure out how to do next.

But in general, this solution does exactly what I wanted it to do: it triggers some functionality before the ready hook, solving my problem. Is there a specific reason why it's not eligible for consideration to be merged in this current PR but requires more separate work?

@flozero
Copy link
Collaborator

flozero commented Aug 20, 2019

Ok maybe we should resolve your issue with the infinity loop before so

@Swader
Copy link

Swader commented Aug 20, 2019

The problem with the loop is that the webpackbar is being rendered via a renderloop in CMDER on Windows, but my plugin is using console.log to output info, plus other tools the plugin runs have their own output (git clone, nim doc, etc.). I think the webpackbar gets stuck with all that output trying to re-render itself, because it works if I do it like this: vuepress build > output.log. Then I only get the third-party tool output, but not the vuepress output, and webpackbar doesn't get stuck (it is not available for render in the log file, so never tries to render). Same in Git bash - it cannot render the webpackbar, so it never crashes and works fine. Only cmder which renders it fine will crash if there is some other output. I'm trying to repro now to be sure.

@@ -1,5 +1,22 @@
# Lifecycle

## int
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init

@Swader
Copy link

Swader commented Aug 20, 2019

@shad-k I'm trying to figure out why init doesn't fully complete during my plugin's execution. I did make the function async and you do have an await in there so by all indications it should, but then we have this:

image

Notice the "writing to file" - those are mine, happening in the plugin which is hooked into init. The server should not start rendering half way through the plugin's operation, but it does. 🤔

Debugging further...

@shad-k
Copy link
Author

shad-k commented Aug 21, 2019

@Swader can I see the code where you use the init hook?

@Swader
Copy link

Swader commented Aug 22, 2019

@shad-k
Copy link
Author

shad-k commented Aug 24, 2019

@shad-k instead of ready here: https://github.com/status-im/nimbus-docs-suite/blob/master/docgen/.vuepress/docgen/plugin.js#L11

@Swader I am not very sure of what might be going wrong, but I guess you might need to return a Promise from processReadme and maybe add all the invocations of processReadme to a Promise.all. This is just from a very brief look at your code. I'll dig further.
Let me know if you have a breakthrough.

@Swader
Copy link

Swader commented Aug 25, 2019

@shad-k I couldn't figure it out. In the end I just extracted the plugin functionality outside and ran the build with a new script that does node prepare && vuepress build, got same effect. You still get the bounty, no worries.

@f3ltron this can be closed.

@flozero
Copy link
Collaborator

flozero commented Aug 25, 2019

Thank's @shad-k if you think it should be reopen explain why on this thread thx !

@flozero flozero closed this Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants