feat: Add native node esm module exports

[perrin4869]

I am migrating some of my code to native node ESM modules, but I got stuck due to incompatibilities importing react-router in node.js and in client code.
Doing SSR, I would have to do something like:

import ReactRouter from 'react-router-dom';
const { Link } = ReactRouter;

This is because the import will point to the cjs build, and it does not support named-exports.
The code above in the client side will import the default export for ReactRouter, and I have no way to get the named imports from it, like Link.

This PR fixes the issue by introducing a clone of index.js as index.mjs and exposes it using the exports field of package.json. I do not believe this would cause breaking changes to end users.

Thanks!

[mjackson]

FWIW we already tried to do this before we launched v6 and ran into some issues with it. I can't remember exactly what they were, but @jacob-ebey and @mcansh spent probably 2 days trying to figure out why esbuild was including 2 copies of the router in Remix. I'd be very cautious about merging something like this.

[mcansh]

FWIW we already tried to do this before we launched v6 and ran into some issues with it. I can't remember exactly what they were, but @jacob-ebey and @mcansh spent probably 2 days trying to figure out why esbuild was including 2 copies of the router in Remix. I'd be very cautious about merging something like this.

i'm almost wondering if that was partly due to having it as a regular dependency in react router, but also having it installed as a dependency in Remix apps 🤔 - because even after reverting the esm changes, we still had duplicate copies floating around.

as an aside, i think shipping esm would be a breaking change and thus React Router v7 🤪

[perrin4869]

Yeah, these kind of changes can be very tricky for sure, totally get cautious... but sooner or later I imagine this will need to be solved?
As it stands it is a blocker to anyone trying to do SSR using esm modules, unless they duplicated their source files, one for frontend and one for backend... which is not practical.
The duplicate dependencies indeed doesn't sound like it would be caused by a change like this... The one problem that is known to be caused is that sometimes in node.js you could end up pulling both the commonjs modules and the esm modules...
I don't think this is a breaking change, just a new feature...

[perrin4869]

Slightly updated the exports to be more conservative and target the node environment explicitly.
Please tell me if there's anything else I can do to move this forward!

[mjackson]

it is a blocker to anyone trying to do SSR using esm modules

Isn't React itself blocking this until they ship ESM?

Honestly this doesn't feel like it's very high on the priority list for us given that we already had a bad experience with it and React itself doesn't even ship ESM.

I don't think this is a breaking change, just a new feature

Agree this doesn't feel like a breaking change. Why do you think it's breaking, @mcansh?

[perrin4869]

react and react-dom load under esm without any issues. Even named exports work as expected. I think it's because of the work done on https://github.com/nodejs/cjs-module-lexer - node is doing static parsing on cjs files, looking for module.exports.foo = "..." etc. It got some nice sophisticated features...
You can confirm it by running:

❯ cat foo.mjs
import { createElement } from 'react';

console.log(createElement);

As mentioned before, the bad experiences you mentioned do not seem related to this change...
I don't think this feature is so complicated that you can say it's "not on the priority list" ;)

EDIT: In my personal project, react-router is literally the only package that is not loading under esm...
EDIT2: added package.json to the exports field because yarn will warn if it is not included:

warn Package react-router-dom has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports"

[mcansh]

Agree this doesn't feel like a breaking change. Why do you think it's breaking, @mcansh?

sorry, y'all are right, i was thinking of just migrating to straight esm without still having commonjs for some reason

[ryanflorence]

Sorry, I've been avoiding module compatibility for many years, can you help me understand why we need two ESM builds? Can we accomplish this with the package.json config alone?

[perrin4869]

Well, you are right, we only really need one build of the esm (the mjs one, or add type: "module" to package.json and change the cjs build instead). I left them both there to reduce the number of changes though... maybe someone is using the index.js esm build specifically in their build process...

[perrin4869]

I think the better approach here would be to add type: module instead and change the name of the commonjs build to cjs (that's what I did in my own npm packages), and that would still work the same as far as the API goes, but you'd risk breaking someone's build pipeline

[ryanflorence]

Why don't we try that? Then we can publish an experimental release and see how many things falls apart with it, and if it's a disaster, we can do the double builds.

[perrin4869]

Tried this approach! Hoping I didn't miss anything
I can't find where I can sign the remix cla, link appears to be broken

[mcansh]

I can't find where I can sign the remix cla, link appears to be broken

contributors file: https://github.com/perrin4869/react-router/blob/main/contributors.yml
CLA: https://github.com/perrin4869/react-router/blob/main/docs/contributing.md

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[perrin4869]

@ryanflorence I just cleaned up the PR and fixed the tests
I really wanted to add regression tests for the exports field in here, but unfortunately that is blocked by jestjs/jest#12439. I left the code for those tests, but I commented it out in case in the future there is a way to run those successfully.

[perrin4869]

Just wanted to note that react@18 added the exports field to their package.json file as well.
Here is how they did it.
Is there a reason this isn't merged yet?

[ryanflorence]

We did some work a while back to make sure React Router and History both load natively in an ESM module, this works for me today with a native node es module:

import { StaticRouter } from "react-router-dom/server.mjs";
import { BrowserRouter } from "react-router-dom";
import { createMemoryHistory } from "history";

console.log(StaticRouter);
console.log(BrowserRouter);
console.log(createMemoryHistory);

I think this solves your problem?

[perrin4869]

Yeah, those changes do solve my issue! Do note though that react is using exports fields now, and doing import { StaticRouter } from "react-router-dom/server.mjs"; is not very nice ergonomics. I've been using patch-package for a while to get the expected react-router-dom/server imports working.

[ryanflorence]

@perrin4869 Glad it's working for you :)

What does your patch package look like today?

[perrin4869]

Just using the bare minimum needed to get my project running:

$ cat patches/react-router-dom+6.3.0.patch
diff --git a/node_modules/react-router-dom/package.json b/node_modules/react-router-dom/package.json
index 1d4ca0e..a4fc8a1 100644
--- a/node_modules/react-router-dom/package.json
+++ b/node_modules/react-router-dom/package.json
@@ -12,6 +12,10 @@
   "main": "./main.js",
   "module": "./index.js",
   "types": "./index.d.ts",
+   "exports": {
+     ".": "./main.js",
+     "./server": "./server.js"
+   },
   "unpkg": "./umd/react-router-dom.production.min.js",
   "dependencies": {
     "react-router": "6.3.0",

The PR here is a much more complete implementation

[wight554]

Would be good to see this merged since esm is getting more and more popular nowadays (especially in vite ecosystem)

[Rutulpatel7077]

Not sure why this has not been merged yet! ?

[gregmartyn]

@Rutulpatel7077 it says there are conflicts that have to be resolved first

[perrin4869]

I've been solving conflicts for many months, but I feel there is no real interest in merging this so I kinda gave up, and also there were changes made in #9017 and others which I didn't look deeply into, but I'd imagine maybe the approach here might need to be changed somewhat...
@ryanflorence what do you think?

[patdx]

@perrin4869 Thank you for the patch-package hint. I ran into a seemingly related issue with React Router v6.4.0, Preact and SSR. React Router was requiring the CJS versions of createContext and useContext but only the ESM versions seem to register correctly. Patch below:

 .../react-router-dom-npm-6.4.0-6eb09f3674.patch   | 15 +++++++++++++++
 .../react-router-npm-6.4.0-b95e890b57.patch       | 14 ++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100644 .yarn/patches/react-router-dom-npm-6.4.0-6eb09f3674.patch
 create mode 100644 .yarn/patches/react-router-npm-6.4.0-b95e890b57.patch

diff --git a/.yarn/patches/react-router-dom-npm-6.4.0-6eb09f3674.patch b/.yarn/patches/react-router-dom-npm-6.4.0-6eb09f3674.patch
new file mode 100644
index 0000000..46299f2
--- /dev/null
+++ b/.yarn/patches/react-router-dom-npm-6.4.0-6eb09f3674.patch
@@ -0,0 +1,15 @@
+diff --git a/package.json b/package.json
+index 9e5daceb8ccb3b31891de4000b0c9d3620dde41a..cd0ec9fa3af104662a4500ac7ad4b8489bfaabf4 100644
+--- a/package.json
++++ b/package.json
+@@ -44,5 +44,10 @@
+   ],
+   "engines": {
+     "node": ">=14"
++  },
++  "type": "module",
++  "exports": {
++    ".": "./dist/index.js",
++    "./server": "./dist/server.mjs"
+   }
+ }
diff --git a/.yarn/patches/react-router-npm-6.4.0-b95e890b57.patch b/.yarn/patches/react-router-npm-6.4.0-b95e890b57.patch
new file mode 100644
index 0000000..c28f772
--- /dev/null
+++ b/.yarn/patches/react-router-npm-6.4.0-b95e890b57.patch
@@ -0,0 +1,14 @@
+diff --git a/package.json b/package.json
+index fba23039473b34557f5b131658fa939f796bfbfa..8cabd79c3b4776bbc38063528417f4f063402637 100644
+--- a/package.json
++++ b/package.json
+@@ -39,5 +39,9 @@
+   ],
+   "engines": {
+     "node": ">=14"
++  },
++  "type": "module",
++  "exports": {
++    ".": "./dist/index.js"
+   }
+ }

Here is the full setup: https://github.com/patdx/preact-react-router-ssr-esm

If react-router and react-router-dom provided proper Node ESM exports I think this patching could be avoided.

[brophdawg11]

Hey folks! I caught up with Michael and Ryan on this and we generally think that any addition of exports should probably be done in a major version to be safe. This aligns with Node's recommendation as well. So we'll keep this on the idea roadmap for a v7, but don't want to risk introducing this in v6 and breaking folks apps.

That being said, it seems the only real issue here is the ergonomics of:

// This requires including the .mjs extension
import { StaticRouter } from "react-router-dom/server.mjs";

Please correct me if I'm mistaken on this and there are other functional issues.

Michael/Ryan both seem to recall the only reason react-router-dom/server was broken out was at one point it was using a node-only API and couldn't be exported through the main bundle. That's not the case anymore though so with ESM working and tree-shaking we're inclined to just export StaticRouter out through the main bundle so you could:

import { StaticRouter } from "react-router-dom";

Let us know if there would still be any ESM issues for your apps if we did that instead, and we'll keep exports in mind for v7. Thanks!

[brophdawg11]

We're doing some house cleaning to start the new year. We switched to a new Open Development process recently so new features should go through that process. Do you mind creating a new Discussion with the Proposals label requesting this feature? Please link to this PR in there so it can be leveraged or re-opened if the proposal gets accepted. Thank you!

[patdx]

we're inclined to just export StaticRouter out through the main bundle

Exporting StaticRouter through the main bundle would help 👍

Let us know if there would still be any ESM issues for your apps if we did that instead, and we'll keep exports in mind for v7. Thanks!

The following simplified example still breaks for me in Node.js and I think it would continue to break even with the proposed StaticRouter change above:

TypeError: Cannot read properties of undefined (reading 'context')
    at exports.useContext (node_modules/preact/hooks/dist/hooks.js:1:2816)
    at useInRouterContext (node_modules/react-router/dist/umd/react-router.development.js:326:29)

To my understanding it is because something inside react-router/react-router-dom imported a CJS module, which subsequently loaded Preact CJS version (hooks.js), even though the direct script imports Preact ESM version (hooks.mjs).

// package.json
{
  "type": "module",
  "dependencies": {
    "preact": "10.13.1",
    "preact-render-to-string": "5.2.6",
    "react-router": "6.8.2",
    "react-router-dom": "6.8.2",
    "react": "npm:@preact/[email protected]",
    "react-dom": "npm:@preact/[email protected]"
  }
}

// main.js
import { h } from "preact"; // node_modules/preact/dist/preact.mjs
import renderToString from "preact-render-to-string"; // node_modules/preact-render-to-string/dist/index.mjs
import { Route, Routes } from "react-router"; // node_modules/react-router/dist/main.js
import { StaticRouter } from "react-router-dom/server.mjs"; // node_modules/react-router-dom/server.mjs

function Home() {
  return h("div", undefined, "Home");
}

const out = renderToString(
  h(
    StaticRouter,
    {
      location: "/home",
    },
    h(Routes, undefined, h(Route, { path: "/home", element: h(Home) }))
  )
);

console.log(out);

[patdx]

We're doing some house cleaning to start the new year. We switched to a new Open Development process recently so new features should go through that process. Do you mind creating a new Discussion with the Proposals label requesting this feature? Please link to this PR in there so it can be leveraged or re-opened if the proposal gets accepted. Thank you!

I created a proposal for the first part of your suggestion here:

#10179