Add exports field to all packages #11675
Conversation
🦋 Changeset detectedLatest commit: 3bf46a7 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 |
packages/remix-node/rollup.config.js
Outdated
| @@ -24,7 +24,7 @@ module.exports = function rollup() { | |||
| return [ | |||
| { | |||
| external: (id) => isBareModuleId(id), | |||
| input: `${SOURCE_DIR}/index.ts`, | |||
| input: [`${SOURCE_DIR}/index.ts`, `${SOURCE_DIR}/install.ts`], | |||
There was a problem hiding this comment.
Now that we have the exports field, we can point to the dist folder and remove the hard-coded install.js and install.d.ts files.
| @@ -23,7 +23,7 @@ module.exports = function rollup() { | |||
| { | |||
| input: `${SOURCE_DIR}/index.ts`, | |||
| output: { | |||
| file: `${OUTPUT_DIR}/index.js`, | |||
| file: `${OUTPUT_DIR}/index.mjs`, | |||
There was a problem hiding this comment.
We have to use the mjs extension now to prevent this file being resolved as CJS via the import field within exports.
packages/remix-node/install.js
Outdated
| /* eslint-disable */ | ||
| "use strict"; | ||
|
|
||
| var globals = require("./dist/globals.js"); | ||
|
|
||
| Object.defineProperty(exports, "__esModule", { value: true }); | ||
|
|
||
| globals.installGlobals(); |
There was a problem hiding this comment.
As mentioned, this file and its types are now built into the dist folder.
markdalgleish
changed the title
packages/remix-serve/package.json
Outdated
| "exports": { | ||
| "./package.json": "./package.json" | ||
| }, |
There was a problem hiding this comment.
This is only being added to prevent reaching into package internals.
There was a problem hiding this comment.
LGTM but I would probably get a 👍 from @jacob-ebey as well since he knows the nuances of this stuff really well
|
@mjackson @jacob-ebey and I chatted yesterday about the ESM/CJS stuff and came up with a gameplan that we think should work for v7 - filed that as a separate task in https://github.com/orgs/remix-run/projects/21/views/1 so I think this can probably get merged and then w'ell tackle that task and we can start experimenting with deployed builds on the nightlies? |
|
I might add a major changeset too just so we have something to catch our eye and elaborate on it a bit in the release notes too as something for folks to be aware of if they hit any bundler issues. |
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/index.mjs", |
There was a problem hiding this comment.
I had to add an ESM build to fix a dual-package-hazard issue. In the upload-test integration test we have an instanceof MaxPartSizeExceededError check that started failing because the app and @react-router/node imported different builds of React Router. It seems the move to using exports triggered Playwright to load the ESM build rather than the CJS build.
The
exportsfield was added toreact-routerandreact-router-domas part of #11669, but this PR standardises this across all packages. I've split this out into a separate PR as a standalone decision.