Prevent tree-shaking of side-effect imports of @u-elements/*

[unekinn]

packages/react/src/components/Accordion/AccordionItem.tsx has the import

import '@u-elements/u-details';

This is preseved when we build the library:

// input: AccordionItem.tsx
import { useMergeRefs } from '@floating-ui/react';
import '@u-elements/u-details';
import cl from 'clsx/lite';
import type { HTMLAttributes, ReactNode } from 'react';
import { forwardRef, useEffect, useRef, useState } from 'react';

// output: AccordionItem.js
'use client';
import { jsx } from 'react/jsx-runtime';
import { useMergeRefs } from '../../node_modules/@floating-ui/react/dist/floating-ui.react.js';
import '../../node_modules/@u-elements/u-details/dist/u-details.js';
import { clsx } from '../../node_modules/clsx/dist/lite.js';
import { forwardRef, useState, useRef, useEffect } from 'react';

However, when a consumer uses @digdir/designsystemet-react, the line

import '../../node_modules/@u-elements/u-details/dist/u-details.js';

is tree-shaked away since we define in "sideEffects": false in our package.json.

We need a way to prevent this from happening.

[eirikbacker]

Hi!

I could alter the u-elements pacakge, but self-defining elements is best practice as elaborated by Lit (web component tooling authored by Google). Google Material 3 web, Adobe Spectrum Web Components and Shoelace (who have been discussing self-defining vs manual defining as well) all uses self-defining elements and are also affected by "sideEffects": false. On the other hand Microsoft Fluent UI web components requires the end user to register themselves (even though Microsofts own web component tooling FAST advices on calling .define in the source code of a custom element)

I can see though that Adobe adds their own sideEffects definition in each package - I'll test generating this in u-elements, yalc-installing it, and see if this is respected when further being imported into Designsystemet 🫰

It is a possibility to add a random export import { noTreeShakePleaseKeepMe } from '@u-elements/u-details' to just ease the process as well. I've been thinking to expose a utility whenDefined('tag-name-here'). Maybe this is a good time to provide this utility, so we can import in React to work around tree shaking? 🤔

[mimarz]

Hi!

I could alter the u-elements pacakge, but self-defining elements is best practice as elaborated by Lit (web component tooling authored by Google). Google Material 3 web, Adobe Spectrum Web Components and Shoelace (who have been discussing self-defining vs manual defining as well) all uses self-defining elements and are also affected by "sideEffects": false. On the other hand Microsoft Fluent UI web components requires the end user to register themselves (even though Microsofts own web component tooling FAST advices on calling .define in the source code of a custom element)

I can see though that Adobe adds their own sideEffects definition in each package - I'll test generating this in u-elements, yalc-installing it, and see if this is respected when further being imported into Designsystemet 🫰

It is a possibility to add a random export import { noTreeShakePleaseKeepMe } from '@u-elements/u-details' to just ease the process as well. I've been thinking to expose a utility whenDefined('tag-name-here'). Maybe this is a good time to provide this utility, so we can import in React to work around tree shaking? 🤔

We got it working the first time with changing it to a having a "define" function.

Isn't this is something that affects everyone using the @u-elements package and want to support tree-shaking? The root cause of this problem is the current mess that are the javascript modules and package managers.

When testing, we first got it working with editing the source code as such downstream:

…

// In u-details.js
function define () {
customElements.define("u-details", UHTMLDetailsElement);
customElements.define("u-summary", UHTMLSummaryElement);
}

export { UHTMLDetailsElement, UHTMLSummaryElement, define };


// In AccordionItem 
import { define } from '@u-elements/u-details'
define();

[eirikbacker]

Turns out not to be a u-elements issue but rather a bundling issue in designsystemet. See further discussion in #2479 ☺️

[mimarz]

Turns out not to be a u-elements issue but rather a bundling issue in designsystemet. See further discussion in #2479 ☺️

This statement is not correct 😅 just because we didn't bundle it with our package doesn't mean thats wrong. Both options are valid in the package making world, with each of its pros and cons.

[unekinn]

just because we didn't bundle it with our package doesn't mean thats wrong. Both options are valid in the package making world, with each of its pros and cons.

I disagree here. I have never seen a modern npm package that bundles its dependencies. Moreover, if you choose to do that, you take the responsibility of correctly marking your side-effects. By saying "sideEffects": false AND bundling a side-effectful library, our package is effectively lying.

[eirikbacker]

Turns out not to be a u-elements issue but rather a bundling issue in designsystemet. See further discussion in #2479 ☺️

This statement is not correct 😅 just because we didn't bundle it with our package doesn't mean thats wrong. Both options are valid in the package making world, with each of its pros and cons.

I also disagree - I have seen u-elements bundled in several projects without having the issue caused in Designsystemet - if we do not allowing our dependencies to have their own package.json definitions intact, we make our project a little a brittle ☺️

[unekinn]

By the way, if we had chosen to keep bundling u-elements, this setting in package/react/package.json actually solves the tree-shaking problem:

  "sideEffects": [
    "**/@u-elements/**"
  ],

As I said, the burden would be on us to do this if we chose to bundle it.

[mimarz]

just because we didn't bundle it with our package doesn't mean thats wrong. Both options are valid in the package making world, with each of its pros and cons.

I disagree here. I have never seen a modern npm package that bundles its dependencies. Moreover, if you choose to do that, you take the responsibility of correctly marking your side-effects. By saying "sideEffects": false AND bundling a side-effectful library, our package is effectively lying.

Now I am confused 😅

I agree, its the best and modern approach, we shouldn't bundle dependencies with our package, which is what we are trying to achieve now with #2479?

My point was that its also valid to bundle dependencies with your package if needed, although less ideal because it can cause unwanted sideffects. I know its what we were doing currently, but it was just a fault with bundling being configured wrong in our rollup.

[eirikbacker]

yeah, u-elements can both be bundled and not bundled - both works ☺️ but if both bundling it and saying "sideEffects": false, we cause problems as we are effectively taking away the ability for our dependencies to declare wether they themselves have side effects or not. As stated in #2477 (comment) - providing web components with self-registration (a side effect) is considered best practice. As @unekinn say, we could work around us bundling dependencies and thus creating sideEffects-issues by adding:

"sideEffects": [
    "**/@u-elements/**"
  ],

but seems we all agree on the approach where we do not bundle dependencies 😊