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

"sideEffects": false not working #10253

Closed
OliverJAsh opened this issue Jan 14, 2020 · 18 comments
Closed

"sideEffects": false not working #10253

OliverJAsh opened this issue Jan 14, 2020 · 18 comments
Labels

Comments

@OliverJAsh
Copy link

Bug report

What is the current behavior?

If the current behavior is a bug, please provide the steps to reproduce.

// package.json
{
  "sideEffects": false,
  "dependencies": {
    "webpack": "^4.39.2",
    "webpack-cli": "^3.3.7"
  }
}
// index.js (entry)
import { identity } from "./test";
console.log(identity);
// test.js
export const identity = x => x;

const myFunction = I => {
  const r = {};
  if (typeof I.map === "function") {
    r.map = () => {};
  }
  return r;
};

const result = myFunction({});
// webpack.config.js
const config = {
  mode: "production",
  entry: './src/index.js',
  output: {
    path: './target',
    filename: 'index.js'
  }
};

module.exports = config;

Now run webpack.

What is the expected behavior?

When I run webpack, I expect the myFunction call and body to removed (because I have specified "sideEffects": false), however they are not. Excerpt below:

})([
  function(e, t, n) {
    "use strict";
    n.r(t);
    (e => {
      const t = {};
      "function" == typeof e.map && (t.map = () => {});
    })({});
    console.log(e => e);
  }
]);

For context, I originally discovered this whilst trying to import from an npm module: import { max } from 'fp-ts/es6/Ord';.

Other relevant information:
webpack version: 4.39.2
Node.js version: 12.8.1
Operating System: N/A
Additional tools:

@sokra
Copy link
Member

sokra commented Jan 14, 2020

The behavior of modules that are flagged as "without sideEffects" is:

When none of the direct exports of the module is used, the execution of the whole module can be omitted.

The flag has no effect on statements in code. It's a module-level behavior.

@OliverJAsh
Copy link
Author

@sokra So is there any way to get webpack to dead code eliminate the myFunction call and body?

@sokra
Copy link
Member

sokra commented Jan 14, 2020

That will also work in webpack. Only in production mode during minimizing

@OliverJAsh
Copy link
Author

@sokra You're right, however it does not work if I make a small change:

test.js:

 const result = /*#__PURE__*/myFunction({});
+const map = result.map;

Output in production mode:

})([
  function(e, t, n) {
    "use strict";
    n.r(t);
    (e => {
      const t = {};
      return "function" == typeof e.map && (t.map = () => {}), t;
    })({}).map;
    console.log(e => e);
  }
]);

Reduced test case: https://github.com/OliverJAsh/tree-shaking-test/tree/fp-ts-ord-rollup-custom-webpack-issue

@sokra
Copy link
Member

sokra commented Jan 14, 2020

Yeah this works as expected result.map could have a side-effect, so it can't be removed. You have multiple options:

  • mark the expression as side-effect-free with /*#__PURE__*/
  • enable the pure_getters terser option, which treats all getters as side-effect-free, but that's unsafe. Getter can have side-effects when a getter function is defined or the "object" is not an object which would throw an error as side-effect.

@sokra
Copy link
Member

sokra commented Jan 14, 2020

Note that even this code:

r.map = () => {};

can potentially have side-effects when a setter is defined on Object.prototype.

@OliverJAsh
Copy link
Author

OliverJAsh commented Jan 14, 2020

  • mark the expression as side-effect-free with /*#__PURE__*/

Like this?

-const map = result.map;
+const map = /*#__PURE__*/result.map;

Sadly that seems to have no effect?

Yeah this works as expected result.map could have a side-effect, so it can't be removed

It's interesting that Rollup doesn't behave the same way 🤔

@sokra
Copy link
Member

sokra commented Jan 14, 2020

It's interesting that Rollup doesn't behave the same way 🤔

hmm, I think for /*#__PURE__*/ it behaves the same. I think this statement is removed by some other mechanism of rollup. It seems to analyse the function somehow and figures out the shape of the return value. The result.map is removed independent of __PURE__.

But also seems like rollup is a little bit to smart here, so result.map is even removed when map is a getter with side-effect like in this example.

@OliverJAsh
Copy link
Author

Sadly that seems to have no effect?

Any ideas on this?

@sokra
Copy link
Member

sokra commented Jan 15, 2020

Maybe the map variable is used?

If it doesn't help, it would be great if you reduce your issue to a small reproducible example. Best put this example into a github repository together with instructions how to get to the problem.

@OliverJAsh
Copy link
Author

OliverJAsh commented Jan 15, 2020

@sokra

Reduced test case: https://github.com/OliverJAsh/tree-shaking-test/tree/fp-ts-ord-rollup-custom-webpack-issue

Run yarn and then the other steps are in the README.md.

Note the link is to the fp-ts-ord-rollup-custom-webpack-issue branch

@sokra
Copy link
Member

sokra commented Jan 15, 2020

hmm... I looked up /*#__PURE__*/ documentation and it seems like this annotation can only be put on function calls. So /*#__PURE__*/result.map is actually not valid as it's not a function call.
/*#__PURE__*/result.map() would be valid and is removed. /*#__PURE__*/(() => result.map)() is also valid and is removed.

@OliverJAsh
Copy link
Author

So there's no way to annotate const map = result.map; as having no side effects? I guess we need to ask for that to be added to Terser then?

@OliverJAsh
Copy link
Author

terser/terser#513

@sokra
Copy link
Member

sokra commented Jan 15, 2020

So there's no way to annotate const map = result.map; as having no side effects?

const map = /*#__PURE__*/(() => result.map)();

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@webpack-bot
Copy link
Contributor

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

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

No branches or pull requests

3 participants