redwoodjs / redwood

The App Framework for Startups
https://redwoodjs.com
MIT License
16.83k stars 964 forks source link

ES modules aren't supported #4074

Open MarkBennett opened 2 years ago

MarkBennett commented 2 years ago

Describe the bug I was trying to use the newest version of node-fetch but in doing so discovered that Redwood does not support require() of ES Modules right now when Redwood tried to compile and start my api service.

To Reproduce Steps to reproduce the behavior:

  1. Install and import a pure ES module (ESM)
  2. Run yarn rw dev
  3. Wait a moment while things start up
  4. See error error when node tries to start the api side

You can see a discussion with further details and sample source on the Redwood Discourse where I discussed this issue with @dthyresson and shared more detailed code samples.

Expected behavior I would expect that ES modules will "just work" given they're getting increasingly popular as people write libraries for node, web, and deno.

Screenshots N/A

Additional context

jtoar commented 2 years ago

Right now Redwood indeed doesn't support ES Modules. We plan to support them but we haven't thoroughly consider what that would entail. Since Redwood isn't consumed by anything downstream, we may actually have an easier time converting to ESM, but it would be pure ESM I think.

From reading the thread, it doesn't look like you actually tried to require an ES Module. Rather, it looks like your ES6 syntax (import fetch from 'node-fetch') was transpiled into a require statement (maybe something like const fetch = require('node-fetch')). But just note that you can't actually require an ES Module:

Using require to load an ES module is not supported because ES modules have asynchronous execution. Instead, use import() to load an ES module from a CommonJS module.

Source: https://nodejs.org/api/esm.html#require.

If you don't mind, can I update the title of the issue to reflect the problem more generally?

MarkBennett commented 2 years ago

If you don't mind, can I update the title of the issue to reflect the problem more generally?

Please! This is above my limited understanding of the issue, so please do what you need @jtoar. Thanks!

MarkBennett commented 2 years ago

From reading the thread, it doesn't look like you actually tried to require an ES Module. Rather, it looks like your ES6 syntax (import fetch from 'node-fetch') was transpiled into a require statement (maybe something like const fetch = require('node-fetch')). But just note that you can't actually require an ES Module:

Are you aware of a way to prevent this transiplation? Or would you say that's the actual issue in this case @jtoar?

jtoar commented 2 years ago

Are you aware of a way to prevent this transiplation?

Yes, it's actually pretty simple. Babel's preset-env preset has an option, modules, which you can set to false to tell it to preserve ES modules. Source: https://babeljs.io/docs/en/babel-preset-env#modules.

would you say that's the actual issue?

Great question! Probably not. Telling Babel not to transpile is actually the easy part.

Chores aren't the main issue, but there are a lot of them, like adding file extensions to relative imports (ES modules don't like bare specifiers, or something like that). More generally, we'd have to go through the whole codebase and refactor things like __dirname.

We haven't prioritized ES modules because we've tried to avoid touching our Babel/Webpack/Jest/Storybook configs as we fix bugs for v1. I'd say that's the main issue. And I'm sure pretty it's taken Jest a while to support ES modules. They're close but I don't think they're quite there yet.

This is something we'll have to do, and want to do moreover. Does that kind of answer your question? We'd appreciate any investigation into this! But it's kind of a gargantuan task πŸ˜…

MarkBennett commented 2 years ago

This is something we'll have to do, and want to do moreover. Does that kind of answer your question? We'd appreciate any investigation into this! But it's kind of a gargantuan task πŸ˜…

How would one go about documenting the work required? Could I change that preset then make a list of what breaks?

Is this issue the best place to track or do we need to make it a meta issue if there are dependencies on things like Jest as you say?

It looks like Jest has an open issue tracking this as you say...

https://github.com/facebook/jest/issues/9430

jtoar commented 2 years ago

Hey @MarkBennett sorry it took me so long to get back to you.

Could I change that preset then make a list of what breaks?

You're certainly welcome to! This is basically how I went about it too. Here's the incomplete list I came up with:

This is definitely an incomplete list, and maybe everything here could be approached differently using something like rollup. The task at hand like you said is to just make sure everything still works, so there's a ton of unknowns.

I'll try to put up a PR I was working on in the next few days for you to check out if you're interested!

jonschlinkert commented 2 years ago
  • refactor or polyfill __filename and __dirname

I've had to do this a lot, figured maybe I could save you 20 seconds:

export const __filename = new URL(import.meta.url).pathname;
export const __dirname = path.dirname(new URL(import.meta.url).pathname);
ebramanti commented 2 years ago

Would be interested in getting this working. A lot of @sindresorhus' packages have now moved to only supporting ESM. I am unable to use https://github.com/sindresorhus/p-map in my project.

dennemark commented 1 year ago

Hi, I also have an ESM only module that I need to use on api side. This creates issues in yarn rw dev as well as yarn rw test. I can pass custom node arguments via script tag in packages.json i.e. : "test": "NODE_OPTIONS=--experimental-vm-modules yarn rw test" or "start": "cross-env NODE_OPTIONS=--experimental-specifier-resolution=node yarn rw dev", and the node options are consumed properly. But unfortunately it still fails. Both scripts cannot consume modules that are ESM only.

It would be great, to have at least a workaround. I had similar issues in an old repo and at least for testing it was super convenient to use vitest - it just works.

If there is anything I can help with, please let me know, what I could do. I.e. set up a repo for this issue.

ebramanti commented 11 months ago

Is there any update on this issue? I am noticing more and more projects migrating to ESM and seems like this will only get more important to solve.

maxall41 commented 11 months ago

I'm running into the same thing. Quite a lot of modules are now ESM and it's becoming a bit hard to work around this issue. Are there any workarounds to get this to work? Or will support for ESM modules be added in the next major revision of redwoodjs?

dennemark commented 8 months ago

Running into this issue again... All @thi.ng/umbrella libraries are esm and therefore not useable :(

It might relate to this issue, that was unfortnately closed in typescript repo: https://github.com/microsoft/TypeScript/issues/43329

typescript transpiles import to require statement, which does not work for es-only modules.

jtoar commented 8 months ago

Hey all, ESM support is something we're actively working on. We don't have a timetable yet, but we'll give a better update after the conference next week.

taivo commented 7 months ago

Hi. Just ran into this issue for @headlessui/react, although for the time being, I think the error can be ignored. Would love to see this moved forward. As in the case of jest being problematic, is switching to vitest being considered?

dennemark commented 6 months ago

Here is a workaround that might suit you in some cases. In my case I am importing tinypool. The workaround is currently needed, since typescript compiles an import to a require function although it should be import. And by creating a dynamic import function with a string, it wont be compiled. Solution can be derived also from similar approaches in the aforementioned issue (https://github.com/microsoft/TypeScript/issues/43329)

export const importDynamic = new Function('modulePath', 'return import(modulePath)');

async function myFunction {
  const { Tinypool } = await importDynamic('tinypool');
  const pool = new Tinypool(...)
}

Only issue though: it is async.

dennemark commented 6 months ago

Typescript 5.3. might be helpful, too, soon: https://devblogs.microsoft.com/typescript/announcing-typescript-5-3/#stable-support-resolution-mode-in-import-types

// Resolve `pkg` as if we were importing with a `require()`
import type { TypeFromRequire } from "pkg" with {
    "resolution-mode": "require"
};

// Resolve `pkg` as if we were importing with an `import`
import type { TypeFromImport } from "pkg" with {
    "resolution-mode": "import"
};
cjreimer commented 2 weeks ago

Hello @jtoar, is there any update on this? We're running into this more and more often where libraries throw errors such as:

The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("compromise")' call instead.

The workaround, as suggested above, is typically to write a dynamic import, but shouldn't we support ECMAScript?

Thanks!

Josh-Walker-GM commented 2 weeks ago

Hey @cjreimer, always awesome to hear from you!

It's something that was on our radar but slipped down in priority a little lately so hasn't been moved forward a lot. Jtoar did do some solid work in this direction though!

Right now we can switch a redwood project over to ESM but we break a certain set of core features - the ones that come to mind right now are jest tests and exec scripts but there are likely more. We have a list somewhere but it escapes me right now.

This is work I have been really wanting to get back to and I will bring it up with the team next week to try to get some time scheduled to push it forward again.

What we could do is release an experimental setup command which would switch a project over to ESM. If we could have great users like yourself with large mature projects test it out and feedback on what breaks then it could help us out. We'll probably have to address a few core breaking changes before we can though - tests especially.

This turned into a bit of an info vomit... Is any of this helpful? I know there might not be anything actionable right now for you. Even just the pressure from users wanting this could help me argue for the time it needs.

cjreimer commented 2 weeks ago

Thanks for the quick response @Josh-Walker-GM !

Yes, I'd appreciate it if this ESM support could be bumped. The dynamic import isn't too bad, but the typescript types with dynamic imports can really be a pain!

I've played with converting to ESM on the web side.... and I've noted that it sort of works, but jest tests are impacted. I forget at the moment, but I think you are right that there are some other things impeding on the api side.

And yes, I would be very open to helping test out the changes. I appreciate the work on all this!

larsvaehrens commented 1 week ago

We would really appreciate this feature as well. We're counting over 100+ await import() in our services.

Also willing to help test changes on our end.

Tobbe commented 1 week ago

We're counting over 100+ await import() in our services.

Ouch πŸ™

Josh did spend some more time on this after the nudge from both of you, so please do know that it's helpful and appreciated when you all keep us honest by commenting on issues like this πŸ™‚

Latest related PR to go is was https://github.com/redwoodjs/redwood/pull/10674 And we have this one in the works https://github.com/redwoodjs/redwood/pull/10362

larsvaehrens commented 1 week ago

We're counting over 100+ await import() in our services.

Ouch πŸ™

It's not as bad as it sounds πŸ˜… We have a few packages in packages/ and one of them shares a lot of code between web, api, and the other packages, so that's where all the dynamic imports come from.

Josh did spend some more time on this after the nudge from both of you, so please do know that it's helpful and appreciated when you all keep us honest by commenting on issues like this πŸ™‚

Appreciate the work and look forward to testing. We also use Vitest for our packages.