Skip to content

Factory method for require dependency in AMDRequireDependenciesBlock #8544

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

Merged
merged 2 commits into from
Dec 30, 2018

Conversation

chuckdumont
Copy link
Contributor

@chuckdumont chuckdumont commented Dec 22, 2018

This is a modified re-submission of #8501 which was rejected because of concerns about the code being too tightly coupled with webpack internals. This submission contains only the change to add the factory method to AMDRequireDependenciesBlock.js. This change is consistent with existing patterns, including:

What kind of change does this PR introduce?
Refactoring in support of code reuse and extensibility

Did you add tests for your changes?
No, not needed. No functional changes.

Does this PR introduce a breaking change?
No

What needs to be documented once your changes are merged?
N/A

N/A

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

Copy link
Member

@ooflorent ooflorent left a comment

Choose a reason for hiding this comment

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

Looks good

@chuckdumont
Copy link
Contributor Author

@ooflorent, thanks for approving. How does this get merged?

@sokra sokra merged commit 2e3e2a0 into webpack:master Dec 30, 2018
@sokra
Copy link
Member

sokra commented Dec 30, 2018

Thanks

Note that subclassing these classes is not part of the public API and may change in minor versions. Fix your webpack version if you are doing that.

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.

4 participants