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

More flexible hook system #2337

Merged
merged 6 commits into from Jul 21, 2019
Merged

More flexible hook system #2337

merged 6 commits into from Jul 21, 2019

Conversation

pksunkara
Copy link
Contributor

@pksunkara pksunkara commented Aug 23, 2018

Fixes #1754, Fixes #1938

Implements the discussion regargding hooks from https://gist.github.com/sodatea/37bc8c36d7d0c285dfea52fbe8ddaf90

@pksunkara
Copy link
Contributor Author

@sodatea Can I get some feedback on this?

@sodatea
Copy link
Member

sodatea commented Sep 25, 2018

I'm currently working on a patch release.
Will look into this later for the next minor version.

@LinusBorg
Copy link
Member

@sodatea Is this still on the rosdmap in your opinion?

@sodatea
Copy link
Member

sodatea commented Dec 10, 2018

@LinusBorg Yeah I’ve talked to Evan before. He may have some additional ideas on the API design.

@pksunkara
Copy link
Contributor Author

I am eager to get this merged.

packages/@vue/cli/__tests__/Generator.spec.js Outdated Show resolved Hide resolved
packages/@vue/cli/lib/Generator.js Outdated Show resolved Hide resolved
packages/@vue/cli/lib/GeneratorAPI.js Outdated Show resolved Hide resolved
packages/@vue/cli/lib/add.js Show resolved Hide resolved
packages/@vue/cli/lib/Generator.js Show resolved Hide resolved
}

if (afterAnyInvokeCbs.length) {
logWithSpinner('⚓', `Running completion hooks from other plugins...`)
Copy link
Member

Choose a reason for hiding this comment

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

Does this include the afterAnyInvoke hook from the currently running generator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From all the current plugins.

Copy link
Member

Choose a reason for hiding this comment

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

But the order of afterAnyInvoke callbacks is first all other plugins' hook, then the current plugin's afterAnyInvoke hook.

The exact result depends on which plugin did you invoke first.
This looks confusing to me.
It is the same kind of non-determinism like npm install. We should avoid it.

Copy link
Member

Choose a reason for hiding this comment

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

Also the log is misleading in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this. Current plugins will run first and then others. The order is now documented.

Copy link
Member

Choose a reason for hiding this comment

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

It no longer triggers other plugins' afterAnyInvoke hook as I tested.

I'm also a little bit skeptical about this approach - this is still non-deterministic IMO.
For example, if a user has two plugins with afterAnyInvoke hook, say, eslint and typescript, the order will be:

  • eslint -> typescript on creation
  • eslint -> typescript if user add eslint first
  • typescript -> eslint if user add typescript first

My idea is to keep the order of afterAnyInvoke in sync with that of plugin loading (for now, it is the key enumeration order in package.json) and then figure out a way to specify plugin order in a separate PR. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I didn't think of it like that. Made it completely deterministic like how you said. Should be working now.

@pksunkara
Copy link
Contributor Author

Updated the PR

@sodatea sodatea merged commit 544faee into vuejs:dev Jul 21, 2019
@vue-bot
Copy link

vue-bot commented Jul 21, 2019

Hey @pksunkara, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

@bodograumann bodograumann mentioned this pull request Jul 24, 2019
19 tasks
sodatea added a commit that referenced this pull request Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin depending on another plugin Hook to act after others plugins invocation
4 participants