Mark build output "sideEffects": false

[CraigMacomber]

Description

During the build we copy package.json files into the output for CJS builds and some ESM builds. Thes files lacked "sideEffects": false resulting in poor tree shaking for any bundle using build output with a copied package.json. This included the tree package ESM build which caused its bundle to bloat.

This package.json copy has been removed from the tree package's esm build, but also update to include "sideEffects": false for builds that use it.

Reviewer Guidance

The review process is outlined on this wiki page.

Copilot reviewed 1 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (5)

• common/build/build-common/src/esm/package.json: Language not supported
• experimental/framework/tree-react-api/package.json: Language not supported
• packages/dds/tree/package.json: Language not supported
• packages/tools/devtools/devtools-test-app/package.json: Language not supported
• packages/tools/devtools/devtools-view/package.json: Language not supported

[jason-ha]

Should/could we instead add sideEffects:false to common/build/build-common/src/esm/package.json?

[CraigMacomber]

Should/could we instead add sideEffects:false to common/build/build-common/src/esm/package.json?

I considered that, however with only one production package using this package.json file, I think having the tree build be special and use this file when nothing else does is confusing and needlessly complicates its build. That would keep its sideEffects block (which might someday be something other than false as it can list files and might need to at some point) duplicated in a way thats hard to notice, and harder to update (I'm not sure how we would go about adding files with side effects if using this copy approach: we would need something that copied the relevant subset of the actual main package.json or something.

Just deleting the extra build step that adds redundant and wrong configuration so that all our packages build in a way thats more similar seems like a nice simplification and fix, and future risk reduction for people getting sideEffects wrong. Perhaps I'm missing something, but I don't see the value in keeping that file and its copy into a small subset of our packages during their build.

[jason-ha]

Should/could we instead add sideEffects:false to common/build/build-common/src/esm/package.json?

I considered that, however with only one production package using this package.json file, I think having the tree build be special and use this file when nothing else does is confusing and needlessly complicates its build. That would keep its sideEffects block (which might someday be something other than false as it can list files and might need to at some point) duplicated in a way thats hard to notice, and harder to update (I'm not sure how we would go about adding files with side effects if using this copy approach: we would need something that copied the relevant subset of the actual main package.json or something.

Just deleting the extra build step that adds redundant and wrong configuration so that all our packages build in a way thats more similar seems like a nice simplification and fix, and future risk reduction for people getting sideEffects wrong. Perhaps I'm missing something, but I don't see the value in keeping that file and its copy into a small subset of our packages during their build.

tree isn't meant to be special, but the other "Node16" conversions were generated without using fully symmetric CommonJS+ESM builds. I didn't press the issue since there is/was so much work to do. (I consider this debt.) tree is a complicated (large) package and is a strong canary for anything that might be funny with nested package.json files. (Yay, we found one.) I would rather add some more package.json copying to other packages to get CommonJS + ESM symmetry than remove to have none.
The other debt we have is programmatic support for anything custom with nested package.json (covered by Task 7374: Node16 Debt: Consider integrating 'package.json' emission into fluid-tsc). This includes (now): self-references and sideEffects (where not false). presence has custom nested package.json from self-reference use.
If any package adds side-effects (we intend/hope never AFAIK), then they would just use a custom package.json assuming that debt task is not yet completed.

The value is that we don't have good coverage one CommonJS. We would like CommonJS and ESM outputs to be similar to mitigate trouble with CommonJS uses.

I won't block removal of tree being symmetric, but please restore the other files and then add sideEffect false to the common package.json.

[jason-ha]

I believe this is meaningless. As far as I know this is only relevant to ESM where import and export are used. Right?

[CraigMacomber]

Some tools might use this. I think webpack might if you bundle the common JS code for some reason (maybe for a deployable service bundle). Webpack's documentation about it notes that this annotation is a workaround for non 100% esm stuff, which sounds like it likely has more significant in non-esm stuff. https://webpack.js.org/guides/tree-shaking/

Generally if we want CommonJS and esm to work as similarly as possible it seems like including this is a good idea so we don't get different bundling policies between the tool for any tools that use this setting.

While we don't use webpack or other bundling tools that use this setting on our CommonJS code, that seems like more of an argument for including it (so the thing we test, using this on ESM is more similar to the thing we don't test: bundling CJS which a customer might do)

[jason-ha]

The webpack docs say that only ESM with import/export is supported. Then they warn about translators changing your code.
Since CommonJS can't use ESM, I don't think we are in any danger of this setting being obscurely relevant. Would be nice to at least comment in the description that we don't think it is relevant (since we can't comment in package.json). Ah, I just missed the merge.

[jason-ha]

Please update the PR title and description.
Ideally skip the CJS change unless we think there is a case where it actually comes into play.

[CraigMacomber]

Please update the PR title and description. Ideally skip the CJS change unless we think there is a case where it actually comes into play.

I have updated the description and title. I've leaving the change to the CommonJS modules for now (justification in other thread). This is yet another case of lack of ability to put nice comments in package.json files being annoying...