Make AMD parser modules extensible again #6674
Conversation
|
|
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Hi. It's been 10 days since this PR was posted. I someone going to review it or is there something process step I'm missing? |
| parser.hooks.call.for("define").tap( | ||
| "AMDDefineDependencyParserPlugin", | ||
| this.processCallDefine.bind( | ||
| Object.create(this, { |
There was a problem hiding this comment.
This is weird. Just add the parser as additional argument to call member methods.
|
|
||
| processCallRequire(expr) { | ||
| const processArrayForRequestString = param => { |
There was a problem hiding this comment.
For consistency processArrayForRequestString and processItemForRequestString should also be member methods.
| return true; | ||
| } | ||
|
|
||
| newDefineDependency(...args) { |
There was a problem hiding this comment.
Write out all arguments explicitly.
| newDefineDependency(...args) { | ||
| return new AMDDefineDependency(...args); | ||
| } | ||
| newRequireArrayDependency(...args) { |
There was a problem hiding this comment.
Write out all arguments explicitly.
| newRequireArrayDependency(...args) { | ||
| return new AMDRequireArrayDependency(...args); | ||
| } | ||
| newRequireItemDependency(...args) { |
There was a problem hiding this comment.
Write out all arguments explicitly.
| } | ||
| } | ||
|
|
||
| newRequireDependenciesBlock(...args) { |
There was a problem hiding this comment.
Write out all arguments explicitly.
| newRequireDependenciesBlock(...args) { | ||
| return new AMDRequireDependenciesBlock(...args); | ||
| } | ||
| newRequireItemDependency(...args) { |
There was a problem hiding this comment.
Write out all arguments explicitly.
| newRequireItemDependency(...args) { | ||
| return new AMDRequireItemDependency(...args); | ||
| } | ||
| newRequireArrayDependency(...args) { |
There was a problem hiding this comment.
Write out all arguments explicitly.
| parser.hooks.call.for("require").tap( | ||
| "AMDRequireDependenciesBlockParserPlugin", | ||
| this.processCallRequire.bind( | ||
| Object.create(this, { |
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
|
@chuckdumont Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
@sokra the requested changes have been made. Thanks. |
|
Thanks |
What kind of change does this PR introduce?
Structural (non-functional) changes to the AMD parser modules that restore extensibility which was lost in the v4 upgrade
Did you add tests for your changes?
No. Not needed. No functional changes.
If relevant, link to documentation update:
N/A
Summary
Prior to V4, code reuse in the AMD parser modules could be achieved through the use of the
call require:amd:array,call require:amd:item,call define:amd:arrayandcall define:amd:itemplugin events. These events were removed in V4, with no alternative to facilitate reuse of parser code by plugins. This PR restores extensibility through the use of class inheritance (a cleaner approach for the use case). The extensibility is needed by dojo-webpack-plugin which needs to subclass and provide custom implementations of thieprocessArrayandprocessItemmethods while leveraging the core parser functionality of the base classes.Does this PR introduce a breaking change?
No
Other information
This PR is best reviewed by ignoring whitespace changes.