fix(react): correctly mark dependencies as external

[unekinn]

This ensures Accordion works when consumers have enabled tree-shaking.

Because we didn't mark all our dependencies as external,
import "@u-elements/u-details" was tree-shaked when @digdir/designsystemet-react
was used in an application with tree-shaking enabled.

This happened because we bundled several dependencies (including this one)
with our library, and our library has "sideEffects": false, implying that
any side-effect imports that happened within our library folder was safe to remove.

This was fixed by correctly marking all our dependencies as external, in
which case the application's build will correctly identify "@u-elements/u-details"
as having side-effects and thus not not remove the import.

Closes #2477

[changeset-bot]

🦋 Changeset detected

Latest commit: a5f6ff6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@digdir/designsystemet-react	Patch
@digdir/designsystemet	Patch
@digdir/designsystemet-css	Patch
@digdir/designsystemet-theme	Patch

Not sure what this means? Click here to learn what changesets are.

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

[github-actions]

Preview deployments for this pull request:

Storybook - 20. Sep 2024 - 15:11

Storefront - 20. Sep 2024 - 15:12

Theme - 20. Sep 2024 - 15:12

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	62.09%	4275 / 6885
🔵	Statements	62.09%	4275 / 6885
🔵	Functions	86.15%	168 / 195
🔵	Branches	75.36%	572 / 759

File Coverage

No changed files found.

Generated in workflow #559

[eirikbacker]

Very nice work <3

[mimarz]

Make sure to remove the rollup-plugin-peer-deps-external devDependency from react package.

[mimarz]

Unsure about the test app as-is because it introduces a lot of dependencies we need to maintain in the repository, ontop of having its own configs instead extending the existing one.

My experience has always been to have the test-apps seperate to prevent false positives or stuff bleeds over from being a monorepo.

[mimarz]

What this really means is that we bundle a copy of the dependencies with our library.

This can cause side effects with duplicate packages for our end-users.

[unekinn]

What this really means is that we bundle a copy of the dependencies with our library.

This can cause side effects with duplicate packages for our end-users.

When we don't mark all our dependencies as external, we get stuff like this in our dist/ output:

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

The imports that start with ../../node_modules and the like will refer to our bundled dependencies, while the other imports will refer to the library consumer's installed versions of dependencies.

The side effect of bundling dependencies is that

1. we take away control of the exact version used by the consuming application,
2. we increase the likelihood of the consuming application ending up with multiple versions of the same library -- when we don't bundle dependencies, this will only happen when the application's dependency version is incompatible with the semver range of our dependency version, but when we bundle dependencies it will always happen if the consumer also depends on the library (even if the exact same version)
3. most relevant to the issue at hand: it seems like Vite at least now tree-shakes away side-effectful imports like import '../../node_modules/@u-elements/u-details/dist/u-details.js';, because the bundled dependencies are considered part of our package, and our package.json says "sideEffects": false. Whereas import '@u-elements/u-details/dist/u-details.js'; which is what we get when we correctly mark it as external, will be treated as having side-effects and not tree-shaked away by the vite build in the consuming application.

[mimarz]

What this really means is that we bundle a copy of the dependencies with our library.
This can cause side effects with duplicate packages for our end-users.

When we don't mark all our dependencies as external, we get stuff like this in our dist/ output:

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

The imports that start with ../../node_modules and the like will refer to our bundled dependencies, while the other imports will refer to the library consumer's installed versions of dependencies.

The side effect of bundling dependencies is that

1. we take away control of the exact version used by the consuming application,
2. we increase the likelihood of the consuming application ending up with multiple versions of the same library -- when we don't bundle dependencies, this will only happen when the application's dependency version is incompatible with the semver range of our dependency version, but when we bundle dependencies it will always happen if the consumer also depends on the library (even if the exact same version)
3. most relevant to the issue at hand: it seems like Vite at least now tree-shakes away side-effectful imports like import '../../node_modules/@u-elements/u-details/dist/u-details.js';, because the bundled dependencies are considered part of our package, and our package.json says "sideEffects": false. Whereas import '@u-elements/u-details/dist/u-details.js'; which is what we get when we correctly mark it as external, will be treated as having side-effects and not tree-shaked away by the vite build in the consuming application.

Jepp, bundling dependencies or having users packagemanager install them via dependencies are both valid approaches. They both have their pros and cons as far as I can remember.

I've experienced problems before with bundling dependencies because downstream a different version was used and they were both patching the same global or some other conflicting stuff I can't remember now. This was 5 years ago, so as long as we keep our bundling to a minimum this should be ok.

My point was just that we don't want to bundle a copy of the dependencies unless absolutely needed 🙈

[unekinn]

as long as we keep our bundling to a minimum this should be ok.

To be clear, what this PR is doing is removing all depenency bundling, not adding more dependencies to the bundle

[mimarz]

Great work!

Good thing you put that guard in so that we avoid this being misconfigured again in the future 😓

Suggest also adding a changeset so we can get a specific entry for this in our prerelease changelog incase we need to backtrack due to some other issue.

[mimarz]

Hmm, maybe also update the PR to better reflect the changes done here?

[eirikbacker]

Superb work 🌟