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

Add support for sideEffects hint in resolveId #2593

Closed
lukastaegert opened this issue Dec 13, 2018 · 13 comments · Fixed by #2844
Closed

Add support for sideEffects hint in resolveId #2593

lukastaegert opened this issue Dec 13, 2018 · 13 comments · Fixed by #2844
Assignees

Comments

@lukastaegert
Copy link
Member

Feature Use Case

Taken from #2415 (comment)
Many packages include hints in their package.json files if executing the module without any imports does have any side-effects. Having this information could have a serious positive effect of bundle size while improving bundling performance. As rollup itself does not read package.json files, rollup would need to provide a way for plugins to provide that information.

Feature Proposal

My suggestion is to extend the resolveId hook to have the following form:

type ResolveIdReturnValue = string | false | null | undefined | {id: string | false, sideEffects boolean}
resolveId: (importee: string, importer: string) => ResolveIdReturnValue | Promise<ResolveIdReturnValue>

In case sideEffects: false is provided as additional information, rollup would not include any code from the module unless exports from this module are included. Not sure how easily this could be implemented internally.

@TrySound
Copy link
Member

sideEffects also accepts an array of files with effects
https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

@lukastaegert
Copy link
Member Author

I see, this is good to know. Will need to check if my suggestion will be enough for rollup-plugin-node-resolve to supply the information. In theory, all resolutions should run through the plugin so it should be possible in theory but it might become complicated.

kzc referenced this issue in mischnic/tree-shaking-example Dec 14, 2018
@kzc
Copy link
Contributor

kzc commented Dec 15, 2018

Even for the array sideEffects scenario, I think a boolean would be sufficient to model it from resolve's perspective because it only deals with a single file at a time. It's just an array of micromatch patterns to test on rollup-plugin-node-resolve's side. Every resolve call would independently decide whether the given path is side effect free or not.

The webpack sideEffects implementation throws in a wrinkle in that a file pattern could be that of a custom loader (i.e., css) that emits JS. Here is such an example in openlayers:

  "sideEffects": [
    "proj.js",
    "ol.css"
  ]

I don't know whether Rollup should (or could) try to emulate that behavior for non-JS asset types.

@Jack-Works
Copy link

Does rollup respect "sideEffects": false? I'm using lodash-es, it has this flag, but rollup still emit whole lib.

@lukastaegert
Copy link
Member Author

Not yet, which is what this issue is about. Preliminary work on this has started but it will still take some time.

@ziofat
Copy link

ziofat commented Apr 26, 2019

Hi, does this have an estimated finish date?

I have an issue might relate to this. I created a module with typescript and React, bundled by Rollup. React is peer dependency in this module. TSX syntax have been transpiled to React.createElement(), where React can be loaded through a script tag. This makes React.createElement having side effects.

I know there are several ways to resolve this. But I prefer use sideEffects field to make the module tree-shakable because I cannot remove the script tag on page.

@lukastaegert
Copy link
Member Author

Not really, but the PR on Rollup side is now ready for playing around with it: #2844

What is missing for me is creating a PR for rollup-plugin-node-resolve to see if this works for the intended use case. But #2844 also gives you a user-facing option to control side effects, which should be enough for your use case.

@tonix-tuft
Copy link

tonix-tuft commented Apr 19, 2020

@lukastaegert I am sorry Lukas, I have experienced the following:

I have created a package called library-a bundled with rollup:

  1. library-a depends on library-b:
  // library-a package.json
  ...
  "dependencies": {
    "library-b": "^1.0.0",
    ...
  },
  ...
  1. library-b peer depends on library-c, so I put library-c as a dependency of library-a as well because NPM warns me about it:
  // library-a package.json with both library-b and library-c dependencies
  ...
  "dependencies": {
    "library-b": "^1.0.0",
    "library-c": "^1.0.0"
  },
  ...
  1. library-c has a dependency (not a peer/dev dependency) on library-d (also created by me), hence library-a has a transitive dependency on library-d
    (library-a -> library-c -> library-d):
  // library-c package.json
  ...
  "dependencies": {
    "library-d": "^1.0.0"
  },
  ...
  1. library-d has a file called src/polyfill.js which performs side-effects by modifying Function.prototype.

  2. library-a only uses libraryBFunction1 function of library-b (this function doesn't require library-c) and it doesn't use any of the functionality of library-b which also requires library-c.
    Hence library-a doesn't effectively depend on library-c and as a result library-a doesn't even depend on library-d (but it includes them in node_modules/ because of library-b peer dependency on library-c).

  3. When library-d doesn't have a sideEffects property in package.json and I bundle library-a, I end up with a dist/index.js file that:

    • Contains library-a's functionality (✅ 🆗 , of course, that's the code of the library itself);

    • Also contains only libraryBFunction1 function from library-b (✅ 🆗 , as expected all the other functionality of library-b which is not used by library-a is not bundled into index.js thanks to tree-shaking);

    • Does not contain any of the library-c code at all (✅ 🆗 , as expected, library-c is a dependency of library-b, but as library-a isn't using any of the functions of library-b which in turn require library-c, rollup tree-shakes library-c away and that's correct and good);

    • Also contains all of the code of library-d (💥 Boom! This is not OK ⚠️ , as library-d happens to be dead code because it is only used by library-c, but library-a doesn't even bundle library-c, so bundling library-d's code doesn't make sense at all and should be tree-shaked as well);

Now, if I instead add the sideEffects property to the library-d's package.json:

  // library-d package.json
  ...
  "sideEffects": [
    "./src/polyfill.js"
  ],
  ...

And rebuild library-a, then the dist/index.js generated by rollup doesn't contain library-d code at all, which means that rollup is now tree-shaking library-d correctly and this is great
🎉 🎉 🎉 .

Does this mean that rollup relies on the sideEffects property to determine whether or not to tree-shake the code of an unused module?
Where is this behaviour documented?

How does rollup determine whether a module has side effects or not?
Because in my example, library-c wasn't bundled too (correct), but its package.json didn't define any sideEffects property, so it means that rollup alone has determined that library-c doesn't have any side effects and therefore can be safely tree-shaked (which is correct by the way because indeed it doesn't have side effects).

But for library-d which has a src/polyfill.js side effect, rollup behaved differently:

  • Without sideEffects defined explicitly in package.json of library-d, rollup didn't tree-shake library-d at all and included all of its code into the final dist/index.js of library-a's build, but tree-shaked library-c;

  • With sideEffects defined explicitly in library-d's package.json, rollup tree-shaked library-d away from dist/index.js together with library-c (both are not effectively used and therefore aren't needed at all), and included only the code of library-b really needed by library-a, which is great 😃 and drastically reduces the size of the final dist/index.js bundle.

I know this is a long comment, its aim is to clarify how rollup works under the hood.

Thank you very much for your attention and for rollup.

@lukastaegert
Copy link
Member Author

Rollup does not rely on the side-effects property for tree-shaking. However there are quite a few situations where Rollup is not able to determine if code has side-effects, and your library-d appears to be such an example. The polyfill of course is a side-effect, but there could be other ways the library is written that might be safe but Rollup is not sure about it.

Here the sideEffects property can help, but note that Rollup itself does not know about it, it is rollup-plugin-node-resolve that does. It translates it to the moduleSideEffects module property e.g. in this plugin hook: https://rollupjs.org/guide/en/#resolveid

As an end user, you can control this behaviour yourself by using the treeshake.moduleSideEffects option where you will also find some more explanation how moduleSideEffects work.

Also note that Rollup does not do usually do "module tree-shaking" except via this option. In its core, tree-shaking, at least if you take the origin of the word, has nothing to do with modules but is happening on statement level with the effect, that sometimes whole modules are removed and sometimes partial modules. The fact that library-d is not removed must be caused in how it is written. Maybe it is putting its exports into a global variable? That would be a side-effect that would force the inclusion of everything, and rightfully so.

@tonix-tuft
Copy link

tonix-tuft commented Apr 19, 2020

@lukastaegert Thank you for your reply and for the links.

Maybe it is putting its exports into a global variable?

library-d exports its public API in the following way in its index.js ("module": "index.js" in package.json):

import "./polyfill"; // This is the polyfill mentioned in my comment above (`src/polyfill.js`)
import someFunc from "./someFunc";
import anotherFunc from "./anotherFunc";
import yetAnotherFunc from "./anotherFunc";
...

export default someFunc;
export { someFunc, anotherFunc, yetAnotherFunc };

So in this case, rollup-plugin-node-resolve (i.e. @rollup/plugin-node-resolve) sees that library-d imports ./polyfill and assumes that it is a side effect and without the sideEffects hint has no chance to exclude the whole library-d from the library-a's bundle?

On the other hand, with the sideEffects property, @rollup/plugin-node-resolve is sure that only import "./polyfill"; is a side effect and therefore, as both library-c and library-d are not used by library-a can safely decide not to bundle library-d into library-a's build?

Does it work like this?

Then, library-c is pretty similar, it has an index.js file which looks like this:

import { someFunc } from "library-d"; // Uses stuff from library-d

...

export default aFunction;
export const someObject = {
    ...
};

But as there isn't any import which looks like import "./maybe-some-side-effect.js";, does it mean that @rollup/plugin-node-resolve assumes that library-c does not contain side effects and therefore avoids bundling library-c into library-a's build even if library-c doesn't define sideEffects: false in its own package.json?

This is the thing I am trying to understand. Thank you!

@lukastaegert
Copy link
Member Author

So in this case, rollup-plugin-node-resolve (i.e. @rollup/plugin-node-resolve) sees that library-d imports ./polyfill and assumes that it is a side effect and without the sideEffects hint has no chance to exclude the whole library-d from the library-a's bundle?

Well, except that there is no assuming taking place, especially not by the pugin: @rollup/plugin-node-resolve did not find a sideEffects flag, so it does not add the moduleSideEffects option. Thus Rollup itself undertakes a deep analysis of the code and includes all statements that either have side-effects or where the analysis cannot be sure.

On the other hand, with the sideEffects property, @rollup/plugin-node-resolve is sure that only import "./polyfill"

Well, with the sideEffects property, it adds the flag to signal Rollup: "If no import from this module is used, consider it and all its dependencies as having no side-effects". So essentially this means that we assume "./polyfill" has no side-effects either unless some other code imports it as well.

But as there isn't any import which looks like import "./maybe-some-side-effect.js";

How an import is written and whether or not bindings are imported has nothing to do with side-effects. It just means we do not import a binding. If the sideEffects flag is not specified, Rollup will again do the deep analysis of all statements to find out if there are side-effects.

@lukastaegert
Copy link
Member Author

@tonix-tuft
Copy link

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants