search

jenseng / eslint-plugin-remix-react-routes

Validate routes referenced by <Link> and friends in a Remix app
ISC License
36 stars 4 forks source link

Support importing native ESM of .js file #16

Open kentcdodds opened 1 year ago

kentcdodds commented 1 year ago

Thanks for this package! Can't believe I've waited this long to try it.

My package.json is type: module so I'd like to keep my remix.config.js with the .js extension. Right now I'm getting:

Error loading Remix config, remix-react-routes rules will be skipped: Error: Error loading Remix config at /Users/kentcdodds/code/epicweb-dev/epic-stack/remix.config.js
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/kentcdodds/code/epicweb-dev/epic-stack/remix.config.js from /Users/kentcdodds/code/epicweb-dev/epic-stack/node_modules/@remix-run/dev/dist/config.js not supported.
Instead change the require of remix.config.js in /Users/kentcdodds/code/epicweb-dev/epic-stack/node_modules/@remix-run/dev/dist/config.js to a dynamic import() which is available in all CommonJS modules.
kentcdodds commented 1 year ago

After further digging, it looks like the issue is here: https://github.com/remix-run/remix/pull/6597

onemen commented 1 year ago

it is readConfigWorker.ts that set process.env.NODE_ENV = "test"

https://github.com/jenseng/eslint-plugin-remix-react-routes/blob/b108614c4c02a39e7701dc4b7bb9dc22c920a9a9/src/readConfigWorker.ts#L4-L15

jenseng commented 1 year ago

Yeah @onemen is correct, it's not a bug with Remix per se, rather we're relying on the fact that Remix looks at NODE_ENV to decide whether to import or require, and in this plugin we're forcing the latter so that we can reset the require cache. But of course that won't work if you're using ESM. We could stop setting NODE_ENV and (I think?) it would work for ESM, but it would not pick up any changes to your routes file, unless you restart the process. Maybe that's good enough? 🤷

Ideally though I'd like it to work something like this:

  1. Remix should require if 1. it's being loaded from jest OR 2. type != 'module'. Then we can reliably reset the require cache for the latter
  2. Remix should import with a random query string if 1. it's not being loaded from jest AND 2.type == 'module'.

Then we can avoid setting NODE_ENV (or whatever) here and we'll get a reloading config. On the ESM side it's still not 100% foolproof (e.g. the config module could import another module and that won't get cache busted. also memory leaks), but that is a longstanding ESM problem bigger than Remix or this plugin.

kentcdodds commented 1 year ago

Step 2 is how we're dealing with this: https://github.com/epicweb-dev/epic-stack/blob/f21bbe93ce26b3ff391874a102e8a7c83319be11/server/index.ts#L189

This is how HMR works in browsers and people don't seem to complain about memory leaks there so I think maybe we're ok? I guess time will tell... But I think the point is that without changing this, people can't use this plugin if they're using native ESM.

In any case, I'm pretty sure the solution is to remove the NODE_ENV thing and go what you've proposed.