-
Notifications
You must be signed in to change notification settings - Fork 340
build: updates to support Node ESM #3287
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
Changes from all commits
e1a4e1b
b412357
28fe304
791528e
78e68b9
2badeb8
3ce47f2
d68bc53
101daea
0b7fbd1
f7c0b41
78f8d82
699f8e6
73abf12
5f280cb
57a51ff
7b89fb3
8e76502
1bb87fb
04c8ec6
9eda9e5
8a8e373
dd68577
22c0060
773aee6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
--- | ||
"@aws-amplify/ui-react-core": patch | ||
"@aws-amplify/ui-react": patch | ||
"@aws-amplify/ui": patch | ||
--- | ||
|
||
build: updates to support Node ESM | ||
|
||
Confirmed that both #3155 and #3206 are fixed without having to apply any workaround | ||
|
||
Also, test out the changes with the following frameworks/tools: | ||
|
||
| Name | Tested? | Notes | | ||
|---|---|---| | ||
| Next.js@11, 12, 13 | ✅ | | | ||
| Gatsby | ✅ | Works with ESM. Not support loading CJS build | | ||
| Remix | ✅ | | | ||
| Astro | ✅ | Works with ESM. Not support loading CJS build | | ||
| webpack | ✅ | | | ||
| Vite | ✅ | Works with ESM. Not support loading CJS build | | ||
| Rollup | ✅ | Works with ESM. Not support loading CJS build | | ||
| esbuild | ✅ | | | ||
| Parcel | ✅ | | | ||
| Snowpack | ✅ | Need `--polyfill-node` to fix JS incompatibility in dev mode, but is a known [issue](https://github.com/FredKSchott/snowpack/discussions/718) | |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ | |
"baseUrl": ".", | ||
"declaration": true, | ||
"esModuleInterop": true, | ||
"jsx": "react-jsx", | ||
"jsx": "react", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise errors like following will show up and it is dumb for us to add
|
||
"importHelpers": true, | ||
"moduleResolution": "node", | ||
"noImplicitAny": true, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,15 @@ | |
"name": "@aws-amplify/ui-react", | ||
"version": "4.3.5", | ||
"main": "dist/index.js", | ||
"module": "dist/esm/index.js", | ||
"module": "dist/esm/index.mjs", | ||
"exports": { | ||
".": { | ||
"types": "./dist/types/index.d.ts", | ||
"import": "./dist/esm/index.js", | ||
"import": "./dist/esm/index.mjs", | ||
"require": "./dist/index.js" | ||
}, | ||
"./internal": { | ||
"import": "./dist/esm/internal.js", | ||
"import": "./dist/esm/internal.mjs", | ||
"require": "./dist/internal.js" | ||
}, | ||
"./styles.css": "./dist/styles.css", | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simple ESM test to prevent regression There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nice! |
||
"test:watch": "yarn test:unit:watch", | ||
"test:unit": "jest", | ||
"test:unit:watch": "jest --watch", | ||
|
@@ -79,7 +80,6 @@ | |
} | ||
}, | ||
"devDependencies": { | ||
"@rollup/plugin-commonjs": "^22.0.1", | ||
"@rollup/plugin-typescript": "^8.3.1", | ||
"@size-limit/preset-big-lib": "^7.0.8", | ||
"@svgr/core": "^5.5.0", | ||
|
@@ -123,31 +123,31 @@ | |
"size-limit": [ | ||
{ | ||
"name": "Authenticator", | ||
"path": "dist/esm/index.js", | ||
"path": "dist/esm/index.mjs", | ||
"import": "{ Authenticator }", | ||
"limit": "120 kB" | ||
}, | ||
{ | ||
"name": "Geo", | ||
"path": "dist/esm/index.js", | ||
"path": "dist/esm/index.mjs", | ||
"import": "{ MapView, LocationSearch }", | ||
"limit": "330 kB" | ||
}, | ||
{ | ||
"name": "Storage - FileUploader", | ||
"path": "dist/esm/index.js", | ||
"path": "dist/esm/index.mjs", | ||
"import": "{ FileUploader }", | ||
"limit": "140 kB" | ||
}, | ||
{ | ||
"name": "AccountSettings", | ||
"path": "dist/esm/index.js", | ||
"path": "dist/esm/index.mjs", | ||
"import": "{ AccountSettings }", | ||
"limit": "60 kB" | ||
}, | ||
{ | ||
"name": "InAppMessaging", | ||
"path": "dist/esm/index.js", | ||
"path": "dist/esm/index.mjs", | ||
"import": "{ InAppMessagingProvider, InAppMessageDisplay }", | ||
"limit": "110 kB" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,18 @@ | ||
import * as React from 'react'; | ||
import { DirectionProvider } from '@radix-ui/react-direction'; | ||
import * as RadixDirection from '@radix-ui/react-direction'; | ||
|
||
import { createTheme, Theme, WebTheme } from '@aws-amplify/ui'; | ||
|
||
import { AmplifyContext } from './AmplifyContext'; | ||
|
||
// Radix packages don't support ESM in Node, in some scenarios(e.g. SSR), | ||
// the module will be imported as CommonJS module, in which we have to reference the `default` | ||
let sanitizedRadixDirection = { default: undefined, ...RadixDirection }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to your lodash idea, maybe we could reexport the radix imports from a single file, to prevent adding this workaround to primitive/component files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would extract this to a util function in the follow up PR along with lodash |
||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
sanitizedRadixDirection = | ||
sanitizedRadixDirection.default ?? sanitizedRadixDirection; | ||
const { DirectionProvider } = sanitizedRadixDirection; | ||
|
||
export type ColorMode = 'system' | 'light' | 'dark'; | ||
export type Direction = 'ltr' | 'rtl'; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have to use name space imports for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another reason to get rid of radix when we can... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree |
||
import classNames from 'classnames'; | ||
|
||
import { useDeprecationWarning } from '../../hooks/useDeprecationWarning'; | ||
|
@@ -9,6 +9,13 @@ import { ExpanderProps } from '../types/expander'; | |
import { Primitive } from '../types/view'; | ||
import { splitPrimitiveProps } from '../utils/splitPrimitiveProps'; | ||
|
||
// Radix packages don't support ESM in Node, in some scenarios(e.g. SSR), | ||
// the module will be imported as CommonJS module, in which we have to reference the `default` | ||
let sanitizedAccordion = { default: undefined, ...Accordion }; | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
sanitizedAccordion = sanitizedAccordion.default ?? sanitizedAccordion; | ||
const { Root } = sanitizedAccordion; | ||
|
||
const ExpanderPrimitive: Primitive<ExpanderProps, 'div'> = ( | ||
{ | ||
children, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,21 @@ | ||
import * as React from 'react'; | ||
import classNames from 'classnames'; | ||
import { | ||
DropdownMenu, | ||
DropdownMenuTrigger, | ||
DropdownMenuContent, | ||
} from '@radix-ui/react-dropdown-menu'; | ||
import * as Dropdown from '@radix-ui/react-dropdown-menu'; | ||
|
||
import { ButtonGroup } from '../ButtonGroup'; | ||
import { ComponentClassNames } from '../shared/constants'; | ||
import { IconMenu } from '../Icon/internal'; | ||
import { MenuButton } from './MenuButton'; | ||
import { MenuProps, Primitive } from '../types'; | ||
|
||
// Radix packages don't support ESM in Node, in some scenarios(e.g. SSR), | ||
// the module will be imported as CommonJS module, in which we have to reference the `default` | ||
let sanitizedDropdown = { default: undefined, ...Dropdown }; | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
sanitizedDropdown = sanitizedDropdown.default ?? sanitizedDropdown; | ||
const { DropdownMenu, DropdownMenuTrigger, DropdownMenuContent } = | ||
sanitizedDropdown; | ||
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, is this eslint comment avoidable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not allow me to do that because there is a chance There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To add to this point, writing in this way is the less eslint comment way I found 😈 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. For my understanding, when will That conditional was confusing to me, because I was thinking that it would always be one or the other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh radix packages do not fully support Node ESM and it may be imported as CJS format in some cases. When the radix module was imported as CJS via name space import, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it, makes sense to me now, thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zchenwei But we could also add a comment linking the the radix issue which could allow us to removed this workaround? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I already have a comment there. Refresh? I would have a follow up PR to extract this workaround to a util function, I do not want to add too much work in this PR |
||
|
||
export const MENU_TRIGGER_TEST_ID = 'amplify-menu-trigger-test-id'; | ||
export const MENU_ITEMS_GROUP_TEST_ID = 'amplify-menu-items-group-test-id'; | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.