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

Migrate packages to ESM #2061

Merged
merged 24 commits into from
Apr 29, 2023
Merged

Migrate packages to ESM #2061

merged 24 commits into from
Apr 29, 2023

Conversation

connor-baer
Copy link
Member

@connor-baer connor-baer commented Apr 24, 2023

Purpose

While testing out #2020, I discovered that some required dependencies are full ESM packages. ESM cannot be imported into CJS, so we need to migrate Circuit UI to ESM to be able to use web components. This migration is inevitable anyway, and the ecosystem seems ready enough at this point (especially after #2053).

Sindre Sorhus has published an extremely helpful guide for pure ESM packages which I referenced throughout the migration.

Note:
The changes below affect the @sumup/circuit-ui, @sumup/design-tokens, and @sumup/icons packages. @sumup/eslint-plugin-circuit-ui has to remain CJS until ESLint adds support for ESM plugins (see eslint/eslint#15453). I haven't tested if @sumup/cna-template can be turned into a module.

Approach and changes

  • Module resolution: ESM requires imports to use the full path to a file, including the file name (even for index files) and the file extension. I used eslint-plugin-node's file-extension-in-import rule, global search-and-replace with complex regexes, and lots of sweat and tears to modify all imports. Changed imports make up the vast majority of the diff.
  • package.json fields:
    • Added "type": "module" to all packages:

      A package.json "type" value of "module" tells Node.js to interpret .js files within that package as using ES module syntax.

    • Switched to the "exports" field to configure the package entry point(s):

      The "exports" provides a modern alternative to "main" allowing multiple entry points to be defined, conditional entry resolution support between environments, and preventing any other entry points besides those defined in "exports".

    • Configured the "engines" field to require Node.js 16+, the first maintained version of Node with support for ES modules.
  • Dependencies: CJS dependencies with default exports aren't compatible with ESM out of the box. I resolved to copying the code of the use-previous and use-latest dependencies into the repository and to proxying the default exports of the @emotion/styled and @emotion/is-prop-valid packages.
  • Storybook + Vite: I replaced Webpack with Vite as Storybook's bundler. This gives us ESM support out of the box and allows us to remove Babel entirely.
  • Other changes:

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented Apr 24, 2023

🦋 Changeset detected

Latest commit: df9d81f

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

This PR includes changesets to release 3 packages
Name Type
@sumup/design-tokens Major
@sumup/circuit-ui Major
@sumup/icons Major

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

@vercel
Copy link

vercel bot commented Apr 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
oss-circuit-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 27, 2023 3:20pm

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #2061 (df9d81f) into next (8adb8fe) will increase coverage by 0.15%.
The diff coverage is 99.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #2061      +/-   ##
==========================================
+ Coverage   96.81%   96.96%   +0.15%     
==========================================
  Files         248      258      +10     
  Lines       22383    23165     +782     
  Branches     2117     2166      +49     
==========================================
+ Hits        21669    22462     +793     
+ Misses        703      697       -6     
+ Partials       11        6       -5     
Impacted Files Coverage Δ
...t-ui/components/Calendar/RangePickerController.tsx 51.42% <0.00%> (ø)
...mponents/CalendarTagTwoStep/CalendarTagTwoStep.tsx 51.03% <80.00%> (-1.04%) ⬇️
...cuit-ui/components/SelectorGroup/SelectorGroup.tsx 94.76% <80.00%> (-0.53%) ⬇️
packages/circuit-ui/components/Anchor/Anchor.tsx 100.00% <100.00%> (ø)
.../circuit-ui/components/AspectRatio/AspectRatio.tsx 100.00% <100.00%> (ø)
...ackages/circuit-ui/components/AspectRatio/index.ts 100.00% <100.00%> (ø)
packages/circuit-ui/components/Avatar/Avatar.tsx 92.30% <100.00%> (-0.55%) ⬇️
packages/circuit-ui/components/Avatar/index.ts 100.00% <100.00%> (ø)
packages/circuit-ui/components/Badge/Badge.tsx 100.00% <100.00%> (ø)
packages/circuit-ui/components/Badge/index.ts 100.00% <100.00%> (ø)
... and 125 more

... and 25 files with indirect coverage changes

@connor-baer connor-baer changed the base branch from next to typescript-all-the-things April 26, 2023 11:26
Base automatically changed from typescript-all-the-things to next April 26, 2023 17:20
import { Theme } from '@sumup/design-tokens';

// eslint-disable-next-line
const styled = (_styled.default || _styled) as CreateStyled;
Copy link
Member Author

Choose a reason for hiding this comment

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

This hack is necessary to make Emotion.js' default exports play nice with ESM. Adapted from emotion-js/emotion#2730 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the ref and the comment! Hopefully we can remove this in the future, when Emotion improves ESM support (or when we drop Emotion)

Copy link
Member Author

Choose a reason for hiding this comment

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

Likely the second 🤞

@connor-baer connor-baer marked this pull request as ready for review April 27, 2023 08:09
@connor-baer connor-baer requested a review from a team as a code owner April 27, 2023 08:09
@connor-baer connor-baer requested review from tareqlol and robinmetral and removed request for a team April 27, 2023 08:09
Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Not gonna lie, I very much skimmed through this and might have missed things (a lot of file import diff, like you said. GitHub should allow using regex to show/hide diff)

This is such an amazing cleanup of old deps that were sticking around, though! Babel! Lodash! ✨ ⭐ 🌟 💫

Side note: I'm getting mildly panicked with the scope of v7, but maybe that's just me? It's going to be a biggie and we might want to think about ways to help consumer apps opt into as many changes as possible. The build changes to support v7 only (ESM, web components if this makes it into v7, etc) are already going to be a big deal depending on the consumer app stack, so let's try to make everything else as easy to adopt as possible 🙂

(PS. "Comment" not "Approve" primarily because I'm wondering about tests for the hooks that moved to the repo.)

.changeset/flat-lamps-glow.md Show resolved Hide resolved
babel.config.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/circuit-ui/hooks/useLatest/useLatest.ts Outdated Show resolved Hide resolved
packages/circuit-ui/package.json Show resolved Hide resolved
import { Theme } from '@sumup/design-tokens';

// eslint-disable-next-line
const styled = (_styled.default || _styled) as CreateStyled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the ref and the comment! Hopefully we can remove this in the future, when Emotion improves ESM support (or when we drop Emotion)

packages/circuit-ui/tsconfig.build.json Outdated Show resolved Hide resolved
packages/icons/package.json Show resolved Hide resolved
packages/icons/package.json Show resolved Hide resolved
@connor-baer
Copy link
Member Author

connor-baer commented Apr 27, 2023

I'm getting mildly panicked with the scope of v7, but maybe that's just me?

It's true that v7 contains many big infrastructure changes. However, the changes in userland should be minimal. Next.js supports ESM out of the box, Jest can be configured with a single line of code to support ESM dependencies.

The other big change to the implementation will be the switch to Linaria, which will only require the global import of a single stylesheet.

Web components will likely be introduced in v7.x as an experiment and will be fully opt-in.

Or perhaps I misunderstood and you're worried about potential bugs, not the work to upgrade?

@connor-baer connor-baer requested a review from robinmetral April 27, 2023 15:26
Copy link
Contributor

@tareqlol tareqlol left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

I'm relieved! Thank you 😌 (and thanks for the new unit test for the hooks!)

@connor-baer connor-baer merged commit bc88242 into next Apr 29, 2023
@connor-baer connor-baer deleted the feature/esm branch April 29, 2023 21:33
This was referenced Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants