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

Webpack optimization bailout due to side effects in rambda.mjs #647

Closed
fatawesome opened this issue Aug 18, 2022 · 14 comments
Closed

Webpack optimization bailout due to side effects in rambda.mjs #647

fatawesome opened this issue Aug 18, 2022 · 14 comments

Comments

@fatawesome
Copy link

fatawesome commented Aug 18, 2022

Hi and thanks for the lib!

While replacing lodash with rambda I found that webpack optimizations cannot be performed due to top-level variables.
Basically, webpack treats variable definitions, such as const adjust = curry(adjustFn) as a side-effect.
The actual error/warning from webpack is

Statement (VariableDeclaration) with side effects in source code at 28:0-31

I've also made a reproduction repo - https://github.com/fatawesome/rambda-side-effects-example
What do you think on this issue?

@selfrefactor
Copy link
Owner

I will check. Thanks for report and the example repo.

@selfrefactor
Copy link
Owner

I can see that there is issue with Rambda and your setup. I see that Ramda behaves better. I found that Rambda 5.0.0 is one of the latest versions that works well. I will investigate further.

@selfrefactor
Copy link
Owner

I think I found a solution. I will make a release and ask you to test it out.

selfrefactor pushed a commit that referenced this issue Aug 18, 2022
@fatawesome
Copy link
Author

Fastest reaction i've ever seen <3
Will you make a dev version of the package or I can build from sources?

@selfrefactor
Copy link
Owner

I will release a patch release, but I will do that during the weekend. I will write here once it is released.

@selfrefactor
Copy link
Owner

There will be some delay as I want to improve tree shaking testing as part of this ticket. Otherwise the solution seems simple, but I want to be sure that it will work to other bundlers as well.

@fatawesome
Copy link
Author

Sure thing, take your time
Would be interesting to see how you test the bundle :)

@selfrefactor
Copy link
Owner

I will use your config as base for webpack check and run it against local build of the library. Then, I will output the size of the bundle.

BTW, with your exact configuration, even the simplest library version(one export of simple function as R.add), still triggers warning, so I am unsure how reliable these warnings are. What I see is that the final output size through minification is influenced by declarations in package.json.

@fatawesome
Copy link
Author

Yep, I can see that tree-shaking does not work even if I leave rambda.mjs file as the following

function F() { return false; }
function T() { return true; }
function add(a, b) {
  if (arguments.length === 1) return _b => add(a, _b);
  return Number(a) + Number(b);
}
export { add, F, T }

But with such ^ content I don't get any warnings about VariableDeclaration from webpack, only about my ExpressionStatement in src/index.js.

And as I add smth like

...
function curry(fn, args = []) {
  return (..._args) => (rest => rest.length >= fn.length ? fn(...rest) : curry(fn, rest))([...args, ..._args]);
}
const curriedAdd = curry(add);
export { ..., curriedAdd }

VariableDeclaration warnings are back

@selfrefactor
Copy link
Owner

It will be an error to develop around tree-shaking of particular tool, especially in the context of Vite and Parcel. I use your Webpack config as it proves that something is wrong. From what I saw, the warning are not fully correct for me, but I don't intend to dig too deep there. I mostly worry about the oblivious(as wanting too much is often a bad direction), i.e. the size of the bundle. Using minification warnings is worth only if the other tools also provide similar tools. If it is only for a webpack plugin, then I will ignore it as it can be issue with the plugin or webpack itself.

@fatawesome
Copy link
Author

fatawesome commented Aug 23, 2022

Yep, you're right here, I totally agree. If the problem with bailout warnings still has place after this issue resolution (with whatever outcome), I will try other webpack optimization techniques to see if there is a problem with Terser plugin.

But if it defines "side-effect" as it is defined in the scientific field, then he is kinda right in his conclusions.

@selfrefactor
Copy link
Owner

I think I found the solution. I will keep the issue open until the release is made and you confirm that it is fixed.

@fatawesome
Copy link
Author

fatawesome commented Nov 15, 2022

Hello! Sorry for delayed response, github decided not notify me with an email for some reason :/

Yep, tree-shaking fix definitely works. I've commited an updated stats.json to the reproduction repo. It now shows only 1 rambda side-effect, but it does not affect tree-shaking.

@selfrefactor
Copy link
Owner

thanks for the feedback @fatawesome

it helps to confirm that it works. I still have a work to do to create a process out of this, so I will close the issue as it solves the problem.

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

No branches or pull requests

2 participants