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

Support Node ESM #3630

Merged
merged 56 commits into from
Nov 22, 2022
Merged

Support Node ESM #3630

merged 56 commits into from
Nov 22, 2022

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Oct 12, 2022

Closes #2881

I've updated the NextJS test app to use ESM. There is a comment on a commit 5dd5f72#comments that was built against our local packages in the branch. This should be sufficient to say that we support ESM now with the other changes in this PR.

I've included a circleci test to run against all our built packages with esm

This is old, I'm leaving it in to explain some things we considered
This brought up some other issues in order to get it working. All our icon imports needed to have the '.js' suffix and also needed to be imported correctly from packages and not direct from 'src' directories.
I needed to update node so I could write a CSS loader for NodeJS. This resulted in some tests getting updated because the Intl formatting was updated by Node. I've matched it against current Chrome implementation and it matches.
It also required me to make it ok in the linter for package circular dependencies where the circle is just itself, this is seen in react-aria/i18n, it'll be checked in the verdaccio step, though may cause some issues for yarn 2 or pnpm.

BLOCKED BY #3706

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test apps generated with published packages, last time they were built is on this commit 5dd5f72#comments
commits after that were unrelated and should not affect this

Test apps generated in normal build comment

You can also verify these locally, if you're running the esm test, then you must be on node > 16.18. Though the above comments should take care of anything you'd do locally.

🧢 Your Project:

@adobe-bot
Copy link

@adobe-bot
Copy link

@vstreame
Copy link

vstreame commented Oct 12, 2022

@snowystinger Thanks for doing this! BTW on my local patch-package, this is what I did:

diff --git a/dist/module.js b/dist/module.mjs
similarity index 99%
rename from dist/module.js
rename to dist/module.mjs
index 363c04eaa6f97ac44194025a6456829d02bc1210..6c0f9b3a6704d4b3507bd4945a0a21527d0a09fb 100644
--- a/dist/module.js
+++ b/dist/module.mjs
@@ -695,4 +695,4 @@ function $fdbe26a36ce1c672$export$736bf165441b18c7() {
 
 
 export {$fd933927dbac1f15$export$46d604dce8bf8724 as shouldKeepSpectrumClassNames, $fd933927dbac1f15$export$f9d3bfd10703eb31 as keepSpectrumClassNames, $fd933927dbac1f15$export$ce4ab0c55987d1ff as classNames, $bde65b0159e7c06e$export$a5f5a6912b18861c as getWrappedElement, $3df547e395c4522f$export$32d5543ab307c01 as useMediaQuery, $98e5a8ae0e6415af$export$a5795cc979dfae80 as createDOMRef, $98e5a8ae0e6415af$export$79d69eee6ae4b329 as createFocusableRef, $98e5a8ae0e6415af$export$c2c55ef9111cafd8 as useDOMRef, $98e5a8ae0e6415af$export$96a734597687c040 as useFocusableRef, $98e5a8ae0e6415af$export$c7e28c72a4823176 as unwrapDOMRef, $98e5a8ae0e6415af$export$1d5cc31d9d8df817 as useUnwrapDOMRef, $380ed8f3903c3931$export$fe9c6e915565b4e8 as baseStyleProps, $380ed8f3903c3931$export$e0705d1a55f297c as viewStyleProps, $380ed8f3903c3931$export$abc24f5b99744ea6 as dimensionValue, $380ed8f3903c3931$export$f348bec194f2e6b5 as responsiveDimensionValue, $380ed8f3903c3931$export$f3c39bb9534218d0 as convertStyleProps, $380ed8f3903c3931$export$b8e6fb9d2dff3f41 as useStyleProps, $380ed8f3903c3931$export$46b6c81d11d2c30a as passthroughStyle, $380ed8f3903c3931$export$52dbfdbe1b2c3541 as getResponsiveProp, $59d09bcc83651bf9$export$1e5c9e6e4e15efe3 as useSlotProps, $59d09bcc83651bf9$export$365cf34cda9978e2 as cssModuleToSlots, $59d09bcc83651bf9$export$8107b24b91795686 as SlotProvider, $59d09bcc83651bf9$export$ceb145244332b7a2 as ClearSlots, $54cda195bd4173fb$export$e52e2242b6d0f1d4 as useHasChild, $fdbe26a36ce1c672$export$736bf165441b18c7 as useIsMobileDevice, $857d64dbfd73d664$re_export$useValueEffect as useValueEffect, $1051245f87c5981d$export$8214320346cf5104 as BreakpointProvider, $1051245f87c5981d$export$140ae7baa51cca23 as useMatchedBreakpoints, $1051245f87c5981d$export$199d6754bdf4e1e3 as useBreakpoint, $857d64dbfd73d664$re_export$useResizeObserver as useResizeObserver};
-//# sourceMappingURL=module.js.map
+//# sourceMappingURL=module.mjs.map
diff --git a/dist/module.js.map b/dist/module.mjs.map
similarity index 100%
rename from dist/module.js.map
rename to dist/module.mjs.map
diff --git a/package.json b/package.json
index 7afca4093e0b4bcecc0d9a10b9639d7e3ce8dad4..85f3bd6e0b20c3b16651e38fc37215b39dcacc9e 100644
--- a/package.json
+++ b/package.json
@@ -4,7 +4,11 @@
   "description": "Spectrum UI components in React",
   "license": "Apache-2.0",
   "main": "dist/main.js",
-  "module": "dist/module.js",
+  "module": "dist/module.mjs",
+  "exports": {
+    "import": "./dist/module.mjs",
+    "require": "./dist/main.js"
+  },
   "types": "dist/types.d.ts",
   "source": "src/index.ts",
   "files": [

By making the module file end with .mjs as well as adding the conditional export all of the ESM loading was fixed both for NodeJS as well as continued to work for ESBuild

@adobe-bot
Copy link

@snowystinger snowystinger marked this pull request as ready for review October 13, 2022 00:08
@adobe-bot
Copy link

@adobe-bot
Copy link

reidbarber
reidbarber previously approved these changes Oct 13, 2022
@adobe-bot
Copy link

reidbarber
reidbarber previously approved these changes Oct 13, 2022
ktabors
ktabors previously approved these changes Oct 14, 2022
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

LGTM, the supportESM.js seems to be the script made to update all the package.json files and is around to run in case we forget to manually do this on new packages in the future, right?

.circleci/comment.js Outdated Show resolved Hide resolved
examples/rsp-next-ts/next.config.mjs Show resolved Hide resolved
meta: {
fixable: 'code'
},
create: function (context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

no actual change to this logic, it's just indented one level and wrapped differently to match new eslint requirements

@@ -11,7 +11,7 @@
*/

import {Badge} from '../';
import CheckmarkCircle from '@spectrum-icons/workflow/src/CheckmarkCircle';
import CheckmarkCircle from '@spectrum-icons/workflow/CheckmarkCircle';
Copy link
Member Author

Choose a reason for hiding this comment

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

corrected all instance of imports like this, we should never import icons from the src file

@@ -34,7 +34,7 @@ function template(iconName) {
let jsx = compileSVG(path.join(expressDir, expressName + '.svg'), 'ExpressIcon');

return (
`import {${iconName} as IconComponent} from '@adobe/react-spectrum-ui/dist/${iconName}';
`import {${iconName} as IconComponent} from '@adobe/react-spectrum-ui/dist/${iconName}.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

@adobe/react-spectrum-ui only provides CJS, so we include the file extension to force any loaders to recognize that as opposed to everywhere else where we let the loader choose which one it wants to use

},
});

export async function resolve(specifier, context, next) {
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 is a fake css loader, it's only needed for our esm test build, it does not get used anywhere else

@adobe-bot
Copy link

@adobe-bot
Copy link

reidbarber
reidbarber previously approved these changes Nov 21, 2022
@@ -7,6 +7,11 @@
"main": "dist/main.js",
"module": "dist/module.js",
"types": "dist/types.d.ts",
"exports": {
"types": "dist/types.d.ts",
"import": "dist/module.js",
Copy link
Member

Choose a reason for hiding this comment

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

Small thing: these should be

Suggested change
"import": "dist/module.js",
"import": "dist/module.mjs",

right? Same issue for the other plop templates if so

Copy link
Member Author

Choose a reason for hiding this comment

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

ugh, yep, i'll do here so we can back out if we need to easily

@adobe-bot
Copy link

@adobe-bot
Copy link

@snowystinger snowystinger merged commit 5198442 into main Nov 22, 2022
@snowystinger snowystinger deleted the support-esm branch November 22, 2022 18:07
@vstreame
Copy link

Congrats @snowystinger ! What an amazing feat to pull this off!

@snowystinger
Copy link
Member Author

Adding an update to reduce the size of some of our files
#3810 will comment here again when it's merged

@snowystinger
Copy link
Member Author

#3810 has been merged, it'll be in the nightlies starting tonight

devongovett added a commit that referenced this pull request Dec 12, 2022
This reverts commit 5198442.

# Conflicts:
#	yarn.lock
@devongovett devongovett mentioned this pull request Dec 12, 2022
@devongovett
Copy link
Member

Unfortunately, we had to revert this for now. It broke webpack 4, which still has more downloads per week than webpack 5. See #3839 for details. We may be able to regroup in the new year, but so far we haven't found a solution that works across all tools.

@Pagebakers
Copy link

@devongovett Any chance this is getting picked up again? Not using .mjs for the esm bundle (as per spec) is causing issues as you can read in the mention above.

@snowystinger
Copy link
Member Author

Yes, this is still on our radar and something we'd like to address. We're accepting contributions and ideas on how to address the webpack 4 issue. It may be a sprint or two before we get back to it.

@Pagebakers
Copy link

Pagebakers commented Jan 27, 2023

I read up on the issue in #3839. Seems like the exact same thing I'm running into now with Next.js (and webpack 5). Unfortunately there is only one way forward and that's to use .mjs extensions.

This fixes it for me in Next.js. Not an ideal solution, but it might be necessary for webpack4 users to add a custom config moving forward.

module.exports = {
  transpilePackages: ['@saas-ui/date-picker'],
  // optional
  webpack(config, options) {
    config.module.rules.push({
        type: 'javascript/auto',
        test: /\.mjs$/,
        use: [options.defaultLoaders.babel],
    })
    return config
  }
}

@devongovett
Copy link
Member

I think I got a new approach working in #4038

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to load @react-aria/textfield as ESM module
9 participants