fix(node-resolve): update side effects logic to be deep when glob doesn’t contain /

[TheMcMurder]

Rollup Plugin Name: node-resolve

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

• Issue with packages that specify side effects rollup#4084

Description

node-resolve doesn't include files from nested folders even when they match files in sideEffects.

Example:

root-dir
- index.js
- deep-dir
---- other-file.js

{
  "main": "index.js",
  "sideEffects": ["*.js"]
}

the side-effects of other-file.js will not be included because picomatch only matches items in the root dir unless we change the sideEffects block to include wildcards.

This should fix this issue by doing a deep search when the glob does not contain a /. This matches the same behaviour as webpack.

[tjenkinson]

👋 I’m wondering if this is this something we should do or should packages themselves already be including **/ if they also want to match children?

[TheMcMurder]

👋 I’m wondering if this is this something we should do or should packages themselves already be including **/ if they also want to match children?

I've debated this as well. The disconnect for me is that, in my testing, nested files are included in webpack and parceljs without adding **/ to sideEffects.

I'll be honest and say I'm not sure if that alone is a good reason to change it. I couldn't find the official spec for sideEffects so I wasn't sure if webpack/parcel was doing it incorrectly or this plugin was doing it incorrectly.

I chose to start here because I use rollup more out of preference and am (slightly) more familiar with the codebase.

I debated several possible fixes but in the end this one seemed simpliest

[tjenkinson]

Hmm yeh I think I’ve looked for a spec in the past and there might not be one :/

It’s a shame we aren’t already in line with the webpack/parcel implementations.

Maybe this is something where we should add a flag like you proposed and in the next major version make it true by default to being us inline with the other bundlers?

One concern I have with the flag at the moment is that it applies to everything, so you could end up with an annoying problem where one dependency needs it to be true to work and another only works with false. Maybe the chance of that is so unlikely it’s not worth worrying about though?

[TheMcMurder]

I'm okay with making it the default behavior over time, I wanted to make sure it was a non-breaking change when introduced.

One concern I have with the flag at the moment is that it applies to everything, so you could end up with an annoying problem where one dependency needs it to be true to work and another only works with false. Maybe the chance of that is so unlikely it’s not worth worrying about though?

I hadn't even thought about that scenario. How could we address something like that? I don't want to add even more configuration but my first idea goes to another configuration item:

// Brainstorming
// assuming it's default behavior in the future

  const bundle = await rollup({
    input: 'deep-side-effects/index.js',
    onwarn: failOnWarn(t),
    plugins: [
      nodeResolve({
        rootDir: 'deep-side-effects',
        ignoreDeepEffectsForPackages: [/* list of packages to ignore the deep effects */]
      })
    ]
  });

[tjenkinson]

Yeh that looks like it would work. Imo let’s not worry about it until someone needs it

@shellscape do we have a way of keeping a list of defaults that may change in a future major version? If not maybe in the readme we could say there the default will become true in the future?

On that topic please can you add this option to the readme? Other than that lgtm

[TheMcMurder]

Whoops, good call. I added a brief description to the readme. Happy to edit the description or include an example if you think that would be beneficial.

[tjenkinson]

Looks good. Got another question though after thinking about this a bit more.

If you have the following structure

- a/
-- b.js
-- a/
--- b.js

and wanted /a/b.js to be flagged as having side effects but /a/b/a/b.js should not have side effects how would you represent that?

Do you know if the other bundlers would support it being /a/b.js vs a/b.js

And if that's the case we should probably do the same and not add the **/ suffix if the input starts with /?

[TheMcMurder]

I'm not sure. I'll experiment with how webpack handles it and use that to inform if we should add the **/ suffix. It may take me a few days to experiment with that though. In the meantime I'm going to push a new commit to fix the prettier issues.

[tjenkinson]

@TheMcMurder I just found the webpack docs and they were more specific than I remembered.

The array accepts simple glob patterns to the relevant files. It uses glob-to-regexp under the hood (Supports: *, **, {a,b}, [a-z]). Patterns like .css, which do not include a /, will be treated like **/.css.

https://webpack.js.org/guides/tree-shaking/

That last part is the key and what we should do I think, and then we don't need the extra option.

*.js should become **/*.js, but something like ./*.js should be left alone.

[TheMcMurder]

@tjenkinson fantastic! I'm really glad you found the docs that I failed to find because it enables a much cleaner solution. I updated the MR.

• removed new option and associated docs
• updated tests

[tjenkinson]

Looks great. One comment

[tjenkinson]

Lgtm!

[shellscape]