Skip to content

Improve tree shakability #612

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.

Sign up for GitHub

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

Merged
ExplodingCabbage merged 5 commits into from
May 20, 2025
Merged

Improve tree shakability #612

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
72bcb6c
Add `sideEffects: false` to `libesm/package.json`
fisker May 16, 2025
5b7948c
Make tweaks I proposed in convo at https://github.com/kpdecker/jsdiff…
ExplodingCabbage May 19, 2025
27e04ed
Fix build for Unix (and break it again for Windows in the process)
ExplodingCabbage May 19, 2025
2425402
Use `node --eval`
fisker May 20, 2025
44d0fbf
--target es6 for ESM build
ExplodingCabbage May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@
"clean": "rm -rf libcsm/ libesm/ dist/ coverage/ .nyc_output/",
"lint": "yarn eslint",
"build": "yarn lint && yarn generate-esm && yarn generate-cjs && yarn check-types && yarn run-rollup && yarn run-uglify",
"generate-cjs": "yarn tsc --module commonjs --outDir libcjs && echo '{\"type\": \"commonjs\"}' > libcjs/package.json",
"generate-esm": "yarn tsc --module nodenext --outDir libesm && echo '{\"type\": \"module\"}' > libesm/package.json",
"generate-cjs": "yarn tsc --module commonjs --outDir libcjs && echo {\"type\": \"commonjs\"} > libcjs/package.json",
Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it'll serve a purpose, but I figure we should add "sideEffects": false here too? It's true and could plausibly be useful, since a quick google reveals that tree-shaking plugins for CommonJS do exist. (I have no idea whether they respect "sideEffects": false, but even if they don't today, they might do tomorrow.)

Copy link
Contributor Author

@fisker fisker May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add it since Prettier only use ESM version 😄

"generate-esm": "yarn tsc --module nodenext --outDir libesm && echo {\"type\": \"module\",\"sideEffects\":false} > libesm/package.json",
Copy link
Contributor Author

@fisker fisker May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed single quotes since it doesn't work on Windows.

Copy link
Contributor Author

@fisker fisker May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense also to add --target es6? Class should not be transformed in ESM.

Copy link
Contributor Author

@fisker fisker May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if TypeScript allows use .mjs as extension, better use .mjs and remove the package.json.

Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if TypeScript allows use .mjs as extension, better use .mjs and remove the package.json.

I think it always matches the extension of the files being compiled (so the only way to get .mjs output is to name your source files .mts). At least, I haven't found any other way to tweak the extensions that tsc outputs.

Honestly, I found setting up a TypeScript build pipeline that was compatible with everything and made arethetypeswrong happy to be a total nightmare, but I see that at least some popular TypeScript libraries like Inquirer.js use the same pattern I settled on for jsdiff (having a CJS folder and an ESM folder, using .js extensions in both, having a package.json in each, and duplicating the type declarations across the two of them) so I think what I'm doing here must be idiomatic and non-awful!

Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed single quotes since it doesn't work on Windows.

Huh, interesting - do you personally develop on Windows? It looks to me like your changes here break this for non-Windows machines, since on my machine a package.json command like "generate-cjs": "echo {\"type\": \"commonjs\"} > file actually just writes {type: commonjs} to file (without the double quotes). I don't know CMD syntax well enough to know off the top of my head how to straightforwardly write this as a sh/CMD polyglot.

Are there any widely-accepted norms about how to go about supporting Windows in package.json scripts? It seems to me it'd be simpler to only support one species of shell language; perhaps I should put something in the README advising Windows people to use Git-Bash or WSL? Or I could wrap all shell commands in a sh -c invocation to ensure that, if they're executed at all, it's by sh and not CMD (but that's still only partly useful to Windows users unless they've got sh on their PATH).

Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense also to add --target es6? Class should not be transformed in ESM.

Hmm. Does it do any harm to transform them? They can still be instantiated and can still be extended with extends; is there anything that gets broken by the transformation?

Copy link
Contributor Author

@fisker fisker May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No to all your questions, but it's unnecessary, since ESM version can't run in es5 environment, but it's up to you.

Copy link
Contributor Author

@fisker fisker May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any widely-accepted norms about how to go about supporting Windows in package.json scripts?

Prettier uses node --eval "fs.writeFileSync()", this line was changed many times, funny story.

Copy link
Contributor Author

@fisker fisker May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2425402 (#612)

"check-types": "yarn run-tsd && yarn run-attw",
"test": "nyc yarn _test",
"_test": "yarn build && cross-env NODE_ENV=test yarn run-mocha",
Expand Down Expand Up @@ -127,5 +127,6 @@
"lines": 100,
"functions": 100,
"statements": 100
}
},
"sideEffects": false
Copy link
Contributor Author

@fisker fisker May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No harm to have this in the root package.json.

Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. dist/diff.js and dist/diff.min.js do have side effects - they create the Diff global. Probably this doesn't matter; I think there's no way anybody could be consuming those files that would respect sideEffects: false, given that they're not even exported via our exports property in package.json. But I am not 100% sure of that. If it's not serving a purpose, I am inclined not to add this line here, just in case. (Leaving out the one that doesn't do anything also makes it clear which property is actually having an effect, if we want to change it later.)

}