Better includeNodeModules for the babel configuration

[stof]

The includeNodeModules feature is a nice DX improvement to manage the common case of processing some node_modules packages (in addition to the whole application code). But the current implementation has a few shortcomings:

• it does not properly account for nested node_modules. Including billboard.js would also process a file node_modules/billboard.js/node_modules/d3-shape/index.js, while this file does not actually belong to the billboard.js package
• as it accept strings only, which must match a folder name, it makes it harder to include a big list of related packages if they rely on a shared prefix rather than a namespace:
  • including all material-components-web packages can be done using ['material-components-web', '@material'] as config, thanks to the @material namespace mapping to a folder
  • including all d3 packages (which is necessary as of v6 if you need to target older browsers as they ship ES6 code) would require including d3-*, which currently requires listing them all manually (which is a pain, especially when they are transitive dependencies as new ones might be added in upgrades without noticing)

Here is my proposal:

• properly extract the package name from the path (as we need the last match, we will have to get all matches due to the way regexp matching works, so the BC layer described below does not require lots of extra work to get parent package names)
• match the package name against the list of processed packages. Also match the package namespace alone in case of a namespace package for BC.
• if the BC layer is enabled (we need a way to disable it to opt-out of the warning if the processing of the nested package is not actually wanted, to finish the migration), apply the filtering on parent package names too. If this BC layer makes the file be included, trigger a deprecation warning telling users to include the relevant package name in the list (bonus point: ensure we trigger the warning only once per package name to avoid spamming the output)
• add support for providing string | Regexp in the includeNodeModules option, to allow case like including d3-*. The regex would have to match on the package name (supporting Regexp is much harder in the existing implementation as it never actually extracts the package name, but is straightforward in my proposal). We could even go further and support string | Regexp | function, making it more similar to webpack conditions.
• (optional, to be discussed): deprecate support for matching the namespace name alone, in favor of the Regexp
• in a following Encore semver-breaking release, remove the BC layer (and deprecate or remove entirely the option controlling it)

If you agree with the idea, I can provide the implementation. I already have a prototype locally built as a exclude option in my own webpack.config.js file (my code is currently missing the BC layer and the compat with node 10, but that's quick to add).

What do you think @weaverryan @Lyrkan ?

[Lyrkan]

I agree that the current implementation should be fixed, we can't expect users to know which dependencies will be included based on hoisting.

I already thought about it a while ago, but with a different solution: also including hoisted dependencies. I don't think that's a good idea anymore but my reasoning at that time was that users would probably use this feature after adding a new dependency to their package.json and may not want to check each nested dependency to know which one should be included. Also, it would be really annoying to implement :)

(optional, to be discussed): deprecate support for matching the namespace name alone, in favor of the Regexp

Unless I misunderstood what you meant I have mixed feeling about that point. I think most people will probably expect the function to take a list of package names as it currently does, so only supporting Regexp could be perceived as an unnecessary complexity.

[stof]

I already thought about it a while ago, but with a different solution: also including hoisted dependencies.

that would very quickly become "process all node_modules".

I think most people will probably expect the function to take a list of package names as it currently does, so only supporting Regexp could be perceived as an unnecessary complexity.

I don't want to deprecate passing string. I want to (maybe) deprecate passing the string @material to include all @material/* packages (which currently works as an implementation detail).

[Lyrkan]

I don't want to deprecate passing string. I want to (maybe) deprecate passing the string @material to include all @material/* packages (which currently works as an implementation detail).

My bad then, I totally agree :)

[carsonbot]

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

[carsonbot]

Thank you for this issue. There has not been a lot of activity here for a while. Has this been resolved?

[carsonbot]

Hello? This issue is about to be closed if nobody replies.

[carsonbot]

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!