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

Enable per-root module imports #386

Merged
merged 3 commits into from
Apr 10, 2017
Merged

Enable per-root module imports #386

merged 3 commits into from
Apr 10, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 7, 2017

Closes: #380

This pull request seeks to enable importing by assuming a root path of each entry in the Webpack configuration. This turned out to be hilariously difficult to achieve, eventually resulting in the implementation of a custom Webpack plugin, currently inlined in the Webpack configuration.

Example:

Before

import Dashicon from '../../components/dashicon';

After:

import Dashicon from 'components/dashicon';

Open questions:

  • We could simplify this by merely defining resolve.modules = [ 'editor', 'i18n', 'element', 'blocks', 'node_modules' ], with the downside being that it becomes possible to import Editable from 'components/editable'; (a blocks component) from the editor path.
  • Should we move the Webpack plugin elsewhere? A separate repository? A standalone file?

Testing instructions:

Verify that npm run build results in no module resolution errors.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 7, 2017
resolver.plugin( 'module', ( request, callback ) => {
// Find entry root which contains the requesting path
const resolvePath = entryRoots.find( ( entryRoot ) => {
return request.path.startsWith( entryRoot );
Copy link
Member

Choose a reason for hiding this comment

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

If I'm understanding this correctly, it would break if you had two directories named like prefix and prefix-and-more-stuff - there should be a test for a path separator somewhere.

@nylen
Copy link
Member

nylen commented Apr 10, 2017

I'm surprised that something like this doesn't exist yet. It seems like it would be pretty useful as a standalone plugin. As a step in that direction, I think it would be good to pull the class out into a separate file in this PR.

This also seems complicated enough that it would benefit from some test cases (for example, for the case I described in #386 (comment)). Have you given much thought to how difficult that would be to achieve (probably easier if it's packaged as a standalone library)? I found this article on the subject: https://blog.iansinnott.com/testing-webpack-plugins/

@aduth
Copy link
Member Author

aduth commented Apr 10, 2017

@nylen Thanks for the feedback. I think I'm going to pull this out as a separate module and give a go at incorporating at least some basic testing.

@aduth
Copy link
Member Author

aduth commented Apr 10, 2017

Extracted the Webpack plugin to an external module and updated the branch accordingly.

https://github.com/aduth/resolve-entry-modules-webpack-plugin
https://www.npmjs.com/package/resolve-entry-modules-webpack-plugin

@aduth aduth merged commit 7fdb9f5 into master Apr 10, 2017
@aduth aduth deleted the add/per-module-root branch April 10, 2017 23:39
@mtias mtias added this to Documentation in Plugin Apr 12, 2017
@mtias mtias moved this from Documentation to Done in Plugin Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Set Webpack per-root module imports
2 participants