js-yaml icon indicating copy to clipboard operation
js-yaml copied to clipboard

esm release tree shake is not pure

Open loynoir opened this issue 2 years ago • 7 comments

Reproduce

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

Expected

output only LF

Actual

output 1023 lines

loynoir avatar Jul 24 '21 03:07 loynoir

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

puzrin avatar Jul 24 '21 04:07 puzrin

Google does not shows too much. This is interesting to read: https://github.com/d3/d3/issues/3076#issuecomment-644265124

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?

puzrin avatar Jul 24 '21 05:07 puzrin

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 avatar Jul 24 '21 06:07 loynoir

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.

loynoir avatar Jul 24 '21 06:07 loynoir

@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?

puzrin avatar Jul 29 '21 10:07 puzrin

@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.

https://github.com/nodeca/js-yaml/blob/01d91e5a5e82a6533ec4419260ec035b08ac4888/dist/js-yaml.js#L410

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. https://github.com/nodeca/js-yaml/blob/01d91e5a5e82a6533ec4419260ec035b08ac4888/test/issues/0631.js#L23

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 avatar Jul 29 '21 15:07 loynoir

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 avatar Jul 29 '21 15:07 loynoir