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

esm release tree shake is not pure #631

Open
loynoir opened this issue Jul 24, 2021 · 7 comments · May be fixed by #661
Open

esm release tree shake is not pure #631

loynoir opened this issue Jul 24, 2021 · 7 comments · May be fixed by #661

Comments

@loynoir
Copy link

loynoir commented Jul 24, 2021

Reproduce

echo 'export {} from "./dist/js-yaml.mjs"' | rollup

Expected

output only LF

Actual

output 1023 lines

@puzrin
Copy link
Member

puzrin commented Jul 24, 2021

I'm not familiar with details. Could you explain where to read and why functions should be marked manually (that looks strange)?

@puzrin
Copy link
Member

puzrin commented Jul 24, 2021

Google does not shows too much. This is interesting to read: d3/d3#3076 (comment)

Also:

  • How standard is all that for libraries?
  • Shouldn't we use sideEffects option?
  • All mentioned marks are /*#__PURE__*/, not /*@__PURE__*/. Where to read about difference?

@loynoir
Copy link
Author

loynoir commented Jul 24, 2021

rollup pure
esbuild pure
webpack pure
uglify pure

Both 2 seems equal.
I choose the one has more google result /*@__PURE__*/.
Set sideEffects should work too in most situations.

@loynoir
Copy link
Author

loynoir commented Jul 24, 2021

Scenario

app depends on a standalone bundlized library lib.

lib depends on js-yaml.

Plan A: pure comment

  • Tested: Yes, it works.

  • Plan

    All js-yaml top level function call use magic comment.

  • Pro:

    1. easy to test if app is pure.
    2. will never pollute final output app when comments are kept.
  • Con

    1. need to modify source
  • Why

    lib got pure js-yaml.

    Bundlized lib still have magic comment, so bundlized lib is pure too, even it move place.

Plan B: sideEffects

  • Plan

    Set sideEffects within js-yaml.

  • Pro

    1. no need to modify source
  • Con

    1. not easy to test if app is pure.
    2. may pollute app when lib is standalone file
    3. may pollute app when lib has sideEffects
  • Why

    lib got pure js-yaml.

    But after bundlized, toplevel function call has no magic comments.

@puzrin
Copy link
Member

puzrin commented Jul 29, 2021

@loynoir Sorry for delay with answer. I have no principal objections against marking required methods with comments, as you suggested.

Two more questions prior to move forward (sorry, if stupid, i'm not familiar with topic):

  1. Why some functions of js-yaml stays unremoved? May be we could rewrite those (do some known recipes exist)?
  2. How do you find, which functions should be marked?

@loynoir
Copy link
Author

loynoir commented Jul 29, 2021

@puzrin
I'm no expert. Someday I look some source code, found some magic comments, and did some search. (sorry, forget which project)

I have some thinks, but no guarantee if they are right.

Why some functions of js-yaml stays unremoved?

I think ESM is stateless to make tree-shaking happens , so any might be un-stateless code are kept outside that tree. Eg, toplevel function call.

var str = /*@__PURE__*/new type('tag:yaml.org,2002:str', {

May be we could rewrite those (do some known recipes exist)?

Maybe convert stateful function calls to stateless init functions,
and call them in a big initial function to only keep one __PURE__?

Or just keep multiple __PURE__?

I don't know if there is best practice.

But I'm sure I saw it in some source code, but not generated code, that's how I know this magic comment.

How do you find, which functions should be marked?

echo 'export {} from "./dist/js-yaml.mjs"' | rollup

Besides, I add a test.

input: 'export {} from "./dist/js-yaml.mjs"; export const foo = "bar"; '

Just see the command line output. Those stateful blocks are kept in output.

Besides, remember the location of those stateful thins, and check the output.
To make sure, certain stateful things are not shaked away.

@loynoir
Copy link
Author

loynoir commented Jul 29, 2021

How do you find, which functions should be marked?

For short, I think

  1. top-level code,
  2. ast parser thinks this kind of code can change state.

@loynoir loynoir linked a pull request Mar 21, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants