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

feat(builder): extendPlugins option and builder:extendPlugins' hook #6285

Merged
merged 12 commits into from
Sep 4, 2019

Conversation

galvez
Copy link

@galvez galvez commented Aug 22, 2019

Currently there's no way to determine the final ordering of user-space plugins in relation to plugins registered automatically by modules (and not all modules necessarily use addPlugin(), which uses unshift()). The problem is complex and likely to have other takes, but as @pi0 said:

Screen Shot 2019-08-26 at 17 19 08

So this introduces a new build:extendPlugins configuration hook, similar to extendRoutes(), that allows users to completely modify the order of all plugins before the build.

@galvez galvez marked this pull request as ready for review August 26, 2019 18:11
@@ -225,7 +225,7 @@ export default class Builder {
}

// Plugins
this.plugins = Array.from(this.normalizePlugins())
this.plugins = Array.from(await this.normalizePlugins())
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to do sth. with regards to error handling here?

Copy link
Author

Choose a reason for hiding this comment

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

Don't think so because that would be an user-space error?

@manniL
Copy link
Member

manniL commented Aug 26, 2019

Besides my annotation in the code, I'd like to see a more illuminating description for the PR, e.g., why introducing the change, what benefits, maybe even potential drawbacks.

For other contributors (besides the core team), the description isn't helpful as is and not transparent.

@galvez
Copy link
Author

galvez commented Aug 26, 2019

@manniL agreed, I have updated the description with more info.

@codecov-io
Copy link

codecov-io commented Aug 27, 2019

Codecov Report

Merging #6285 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6285      +/-   ##
==========================================
+ Coverage   95.71%   95.72%   +<.01%     
==========================================
  Files          79       79              
  Lines        2661     2666       +5     
  Branches      681      683       +2     
==========================================
+ Hits         2547     2552       +5     
  Misses         98       98              
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.71% <50%> (-0.03%) ⬇️
#unit 92.38% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
packages/config/src/config/_app.js 100% <ø> (ø) ⬆️
packages/builder/src/builder.js 99.63% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdcf4c8...c6fd67a. Read the comment docs.

pimlie
pimlie previously approved these changes Aug 27, 2019
Copy link

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

Haha, +1 for the discord screenshot of Pooya approving the need for this hook

manniL
manniL previously approved these changes Aug 27, 2019
@atinux atinux self-requested a review August 27, 2019 10:07
atinux
atinux previously approved these changes Aug 27, 2019
@galvez galvez changed the title feat: build:extendPlugins hook feat(builder): add build:extendPlugins hook Aug 28, 2019
@galvez galvez dismissed stale reviews from atinux, manniL, and pimlie via b499baf September 2, 2019 06:43
@pimlie pimlie changed the title feat(builder): add build:extendPlugins hook feat(builder): add extendPlugins hook and options method Sep 4, 2019
@pimlie
Copy link

pimlie commented Sep 4, 2019

By request of @galvez I've added support for options.extendPlugins which follows the same doctrine as options.router.extendRoutes

@pimlie pimlie requested a review from pi0 September 4, 2019 10:45
@pimlie pimlie requested review from atinux and manniL September 4, 2019 10:45
@pi0 pi0 changed the title feat(builder): add extendPlugins hook and options method feat(builder): extendPlugins option and builder:extendPlugins' hook Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants