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

fix: use standard side effects #2469

Closed
wants to merge 2 commits into from
Closed

Conversation

eirikbacker
Copy link
Contributor

@eirikbacker eirikbacker commented Sep 19, 2024

  • Removes non-standard "sideEffects": false from package.json of @digirdir/designsystemet-react
  • This is a property only supported by Webpack and by Vite/Rollup
  • This property caused issues when importing Designsystemet into another project and then building with Vite - aka. an issue that is non-detectable in our own repository
  • Have verified with Øyvind this was just something originally copied from Altinn without further intentions
  • Many thanks to @unekinn for very good pinpong on resolving this issue <3

Vite building with non-standard "sideEffects": false:
image

Vite building when removing non-standard "sideEffects": false:
image

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@eirikbacker eirikbacker self-assigned this Sep 19, 2024
Copy link

changeset-bot bot commented Sep 19, 2024

⚠️ No Changeset found

Latest commit: 808cab9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Sep 19, 2024

Preview deployments for this pull request:

Storybook - 19. Sep 2024 - 16:29

Copy link
Contributor

github-actions bot commented Sep 19, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 62.42% 4337 / 6947
🔵 Statements 62.42% 4337 / 6947
🔵 Functions 86.29% 170 / 197
🔵 Branches 75.52% 574 / 760
File CoverageNo changed files found.
Generated in workflow #514

@eirikbacker eirikbacker requested a review from unekinn September 19, 2024 14:32
@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

Not this rabbithole again 🐰
Does tree-shaking still work?

@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

Just did a quick test with only using Accordion, and removing this causes a false positive. Its working because tree-shaking i no longer working and everything is bundled together 😩

The culprit is that to much is being tree-shaked away, its for example missing @u-elements/u-details which is suspect is causing this for only Accordion?

With sideEffect:false:

image image

Without:

image image

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

The culprit is that to much is being tree-shaked away, its for example missing @u-elements/u-details which is suspect is causing this for only Accordion?

Indeed, this was Eirik's initial finding. My hunch was that maybe @u-elements/u-details needed "sideEffects": true in its package.json. But turns out that is the default behaviour when the property is unset, so it doesn't change anything.

I suspect we might need to specify all files which have side-effectful imports of @u-elements/* in "sideEffects": [...filesWithSideEffects], which is a hassle 😭

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

A second note, setting "sideEffects": false normally shouldn't be required to get working tree-shaking, so I find that behaviour a bit weird. Might there be other reasons why tree-shaking doesn't work? E.g. circular dependencies (very easy to accidentally do with barrel index.ts files)

@Barsnes
Copy link
Member

Barsnes commented Sep 20, 2024

A second note, setting "sideEffects": false normally shouldn't be required to get working tree-shaking, so I find that behaviour a bit weird. Might there be other reasons why tree-shaking doesn't work? E.g. circular dependencies (very easy to accidentally do with barrel index.ts files)

yes we do have some circular dependencies.
They can be seen when building the package

@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

A second note, setting "sideEffects": false normally shouldn't be required to get working tree-shaking, so I find that behaviour a bit weird. Might there be other reasons why tree-shaking doesn't work? E.g. circular dependencies (very easy to accidentally do with barrel index.ts files)

I thought it was need as long as we live in a world with cjs and esm modules? I think most component libraries out there have this in their package.json, due to this reason.

I did fix our circular dependencies during summer, but looks like we got some new ones now 🙈

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

Here's the current circular dependencies in packages/react on next

I'd prefer to set up a check in CI to keep these from popping up

> npx dpdm --no-warning --no-tree src/index.ts 

✔ [174/174] Analyze done!
• Circular Dependencies
  01) src/components/index.ts -> src/components/Button/index.ts -> src/components/Button/Button.tsx
  02) src/components/form/Fieldset/FieldsetContext.ts -> src/components/form/useFormField.ts
  03) src/components/form/Select/Select.tsx -> src/components/form/Select/useSelect.ts
  04) src/components/Pagination/Pagination.tsx -> src/components/Pagination/PaginationButton.tsx -> src/components/Pagination/PaginationRoot.tsx
  05) src/components/Pagination/Pagination.tsx -> src/components/Pagination/usePagination.ts
  06) src/components/form/Checkbox/Checkbox.tsx -> src/components/form/Checkbox/useCheckbox.ts
  07) src/components/form/Radio/Radio.tsx -> src/components/form/Radio/useRadio.ts
  08) src/components/form/Switch/Switch.tsx -> src/components/form/Switch/useSwitch.ts
  09) src/components/form/Textfield/index.ts -> src/components/form/Textfield/Textfield.tsx -> src/components/form/CharacterCounter.tsx
  10) src/components/form/Textfield/Textfield.tsx -> src/components/form/Textfield/useTextfield.ts
  11) src/components/form/Textarea/Textarea.tsx -> src/components/form/Textarea/useTextarea.ts
  12) src/components/ToggleGroup/ToggleGroupItem.tsx -> src/components/ToggleGroup/useToggleGroupitem.ts
  13) src/components/form/Search/Search.tsx -> src/components/form/Search/useSearch.ts
  14) src/components/form/Combobox/Combobox.tsx -> src/components/form/Combobox/ComboboxContext.tsx
  15) src/components/form/Combobox/Combobox.tsx -> src/components/form/Combobox/ComboboxContext.tsx -> src/components/form/Combobox/useCombobox.tsx
  16) src/components/form/Combobox/ComboboxContext.tsx -> src/components/form/Combobox/useCombobox.tsx -> src/components/form/Combobox/Custom.tsx
  17) src/components/form/Combobox/ComboboxContext.tsx -> src/components/form/Combobox/useCombobox.tsx -> src/components/form/Combobox/Option/Option.tsx
  18) src/components/form/Combobox/ComboboxContext.tsx -> src/components/form/Combobox/useCombobox.tsx -> src/components/form/Combobox/Option/Option.tsx -> src/components/form/Combobox/Option/useComboboxOption.tsx
  19) src/components/form/Combobox/Option/Option.tsx -> src/components/form/Combobox/Option/useComboboxOption.tsx -> src/components/form/Combobox/utilities.ts
  20) src/components/form/Combobox/Option/Option.tsx -> src/components/form/Combobox/Option/useComboboxOption.tsx -> src/components/form/Combobox/utilities.ts
  21) src/components/form/Combobox/Combobox.tsx -> src/components/form/Combobox/internal/ComboboxError.tsx
  22) src/components/form/Combobox/Combobox.tsx -> src/components/form/Combobox/internal/ComboboxInput.tsx
  23) src/components/form/Combobox/Combobox.tsx -> src/components/form/Combobox/internal/ComboboxLabel.tsx
  24) src/components/form/Combobox/Combobox.tsx -> src/components/form/Combobox/internal/ComboboxNative.tsx

@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

Here's the current circular dependencies in packages/react on next

I'd prefer to set up a check in CI to keep these from popping up

> npx dpdm --no-warning --no-tree src/index.ts 
…

Interesting, I just used rollups warning during build, which is substantially less 🤔

image

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

As a comparison, here is a report from madge which I have also used before (bonus: also supports css)

> npx madge --extensions ts,tsx ./src --circular

Processed 270 files (1.2s) (8 warnings)

✖ Found 23 circular dependencies!

1) components/index.ts > components/Button/index.ts > components/Button/Button.tsx
2) components/Pagination/Pagination.tsx > components/Pagination/PaginationButton.tsx > components/Pagination/PaginationRoot.tsx
3) components/Pagination/Pagination.tsx > components/Pagination/usePagination.ts
4) components/ToggleGroup/ToggleGroupItem.tsx > components/ToggleGroup/useToggleGroupitem.ts
5) components/form/Checkbox/Checkbox.tsx > components/form/Checkbox/useCheckbox.ts
6) components/form/Fieldset/FieldsetContext.ts > components/form/useFormField.ts
7) components/form/Combobox/Combobox.tsx > components/form/Combobox/ComboboxContext.tsx
8) components/form/Combobox/Combobox.tsx > components/form/Combobox/ComboboxContext.tsx > components/form/Combobox/useCombobox.tsx
9) components/form/Combobox/ComboboxContext.tsx > components/form/Combobox/useCombobox.tsx > components/form/Combobox/Custom.tsx
10) components/form/Combobox/ComboboxContext.tsx > components/form/Combobox/useCombobox.tsx > components/form/Combobox/Option/Option.tsx
11) components/form/Combobox/ComboboxContext.tsx > components/form/Combobox/useCombobox.tsx > components/form/Combobox/Option/Option.tsx > components/form/Combobox/Option/useComboboxOption.tsx
12) components/form/Combobox/Option/Option.tsx > components/form/Combobox/Option/useComboboxOption.tsx > components/form/Combobox/utilities.ts
13) components/form/Combobox/Combobox.tsx > components/form/Combobox/internal/ComboboxError.tsx
14) components/form/Combobox/Combobox.tsx > components/form/Combobox/internal/ComboboxInput.tsx
15) components/form/Combobox/Combobox.tsx > components/form/Combobox/internal/ComboboxLabel.tsx
16) components/form/Combobox/Combobox.tsx > components/form/Combobox/internal/ComboboxNative.tsx
17) components/form/Radio/Radio.tsx > components/form/Radio/useRadio.ts
18) components/form/Search/Search.tsx > components/form/Search/useSearch.ts
19) components/form/Select/Select.tsx > components/form/Select/useSelect.ts
20) components/form/Switch/Switch.tsx > components/form/Switch/useSwitch.ts
21) components/form/CharacterCounter.tsx > components/form/Textfield/index.ts > components/form/Textfield/Textfield.tsx
22) components/form/Textfield/Textfield.tsx > components/form/Textfield/useTextfield.ts
23) components/form/Textarea/Textarea.tsx > components/form/Textarea/useTextarea.ts

@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

Hmm, this definitely needs more testing. I suggest we make a separate issue for this to properly document this.

I would say the root problem here is tree-shaking not working correctly, as to whats the root cause of it remains to be discovered and fixed.

With or without "sideEffects": false, I understood its a common practice used by all component libraries? It should either way work with it enabled to.

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

I thought it was need as long as we live in a world with cjs and esm modules? I think most component libraries out there have this in their package.json, due to this reason.

I think it is necessary for optimal tree shaking, but in the last library I worked on it made a smaller difference than turning tree shaking off entirely. At least with vite. This stuff is complex 😢

Anyhow. If possible, we should keep "sideEffects": false or use the array form to specify the files which have side effects. It would be ideal if this could be solved by u-elements, but I'm not sure how. Currently, each custom element js file has a side effect which does e.g.

customElements.define("u-details", UHTMLDetailsElement);
customElements.define("u-summary", UHTMLSummaryElement);

We could conceivably ask our consumers to do the customElements.define calls, but it's a hassle for them, and it requires knowledge of which of our components use which custom element

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

@eirikbacker Can you check if the code works if we replace "sideEffects": false with this?

  "sideEffects": ["./src/components/Accordion/AccordionItem.tsx"],

If it works, it's annoying to maintain manually. I experimented a bit and managed this magic incantation which finds files that depend on @u-elements, which could be expanded to a script to keep the sideEffects array updated 🤔

> npx madge ./src/index.ts --json --include-npm | npx repdeps -x '(\.\.\/)+node_modules/@u-elements/.+' -r '@u-elements' | npx madge --stdin --depends '@u-elements' --json

output:

[
  "components/Accordion/AccordionItem.tsx"
]

@unekinn
Copy link
Contributor

unekinn commented Sep 20, 2024

Hmm, this definitely needs more testing. I suggest we make a separate issue for this to properly document this.

Created an issue at #2474

@mimarz mimarz mentioned this pull request Sep 20, 2024
@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

Hmm, this definitely needs more testing. I suggest we make a separate issue for this to properly document this.

Created an issue at #2474

Nice! I have added it as part of #2475 as I suspect this will take some time to figure out.

@mimarz
Copy link
Collaborator

mimarz commented Sep 20, 2024

Closing this as we have identified other workarounds and testing downstream shows we still need sideEffects for tree-shaking. Further work is done in #2475

@mimarz mimarz closed this Sep 20, 2024
@mimarz mimarz deleted the fix/use-standard-side-effects branch December 12, 2024 08:07
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 this pull request may close these issues.

None yet

4 participants