build: updates to support Node ESM #3287
Conversation
🦋 Changeset detectedLatest commit: 773aee6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
| @@ -9,7 +9,7 @@ | |||
| "baseUrl": ".", | |||
| "declaration": true, | |||
| "esModuleInterop": true, | |||
| "jsx": "react-jsx", | |||
| "jsx": "react", | |||
There was a problem hiding this comment.
Unfortunately we will have to switch back to old JSX transform because its ESM support only landed in 18 and the team does not plan to backport it to older version <- facebook/react#20304
Unless we drop the support for React 16 and 17, we cannot leverage the new JSX transform in order to support Node ESM
There was a problem hiding this comment.
Otherwise errors like following will show up and it is dumb for us to add .js file extension to every single line of them
Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Volumes/workplace/amplify-ui/packages/react/node_modules/react/jsx-runtime' imported from /Volumes/workplace/amplify-ui/packages/react/dist/esm/components/Authenticator/Authenticator.mjs
Did you mean to import react/node_modules/react/jsx-runtime.js?
| @@ -40,7 +40,8 @@ | |||
| "dev:build": "tsup", | |||
| "clean": "rimraf dist node_modules", | |||
| "lint": "tsc --noEmit --project tsconfig.dev.json && eslint src --ext .js,.ts,.tsx", | |||
| "test": "yarn test:unit", | |||
| "test": "yarn test:unit && yarn test:esm", | |||
| "test:esm": "node --input-type=module --eval 'import \"@aws-amplify/ui-react\"'", | |||
There was a problem hiding this comment.
Simple ESM test to prevent regression
| @@ -1,5 +1,5 @@ | |||
| import * as React from 'react'; | |||
| import { Root } from '@radix-ui/react-accordion'; | |||
| import * as Accordion from '@radix-ui/react-accordion'; | |||
There was a problem hiding this comment.
Have to use name space imports for radix components because it does not support Node ESM and so Node will reference its CJS build(main field) instead(Node does not know module field). Name space imports works in both format.
There was a problem hiding this comment.
Another reason to get rid of radix when we can...
| @@ -1,14 +1,9 @@ | |||
| { | |||
| "ts-node": { | |||
There was a problem hiding this comment.
Do not need this awkward override any more
| @@ -33,7 +33,7 @@ | |||
| "scripts": { | |||
| "prebuild": "rimraf dist", | |||
| "build": "yarn build:css && yarn build:js", | |||
| "build:css": "ts-node --transpile-only ./scripts/generateCSS.ts", | |||
| "build:css": "node -r esbuild-register ./scripts/generateCSS.ts", | |||
There was a problem hiding this comment.
Use esbuild-register to have better ESM support otherwise it will throw error in our ESM build
zchenwei
dismissed stale reviews from calebpollman, reesscot, 0618, and wlee221
via
773aee6
zchenwei
requested review from
dbanksdesign,
0618,
wlee221,
calebpollman and
reesscot
Description of changes
This PR aims to add Node ESM support to
@aws-amplify/ui-reactpackage, following Node ESM spec.Issue #, if available
Fixes #3155
Fixes #3206
Description of how you validated changes
Verified the above mentioned issues are fixed without applying any workaround
Changes are tested with the following frameworks/tools with tagging release
node-esm--polyfill-nodeto fix JS incompatibility in dev mode, but is a known issueChecklist
yarn testpassessideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.