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

Node.js default interop case #532

Closed
guybedford opened this issue Nov 15, 2020 · 26 comments
Closed

Node.js default interop case #532

guybedford opened this issue Nov 15, 2020 · 26 comments

Comments

@guybedford
Copy link
Contributor

I'm posting this here, which is a discussion that came up between Node.js and RollupJS but has grown to include members from other projects as well. See rollup/plugins#635 for the original issue.

@evanw I would really value your feedback here as interop is one of those things where it happens too quickly in a brief moment, and then we are stuck with it for years. Even if the action is do nothing, we need to make sure it is a considered do nothing.

If you have a chance to read that thread or have any questions I would value your input. Perhaps we may just have to live with some thrashing. I still hope not. Best case we get alignment between bundlers.

@evanw
Copy link
Owner

evanw commented Nov 15, 2020

It's very unfortunate that the situation with default is such a mess. I sympathize with the struggle for compatibility, although I'm not sure what the best answer is. Thank you for reaching out about this.

Side note: If we're talking about better interop between bundlers, one thing I'd advocate for is at least behaving consistently regardless of syntax. Right now Webpack, Rollup, and Parcel all expose different exports depending on how you access them syntactically. I did a brief survey of this here: #472 (comment). This makes it difficult for me to know what the most compatible behavior is supposed to be.

I scanned the thread but I'm not too sure what the proposal is. The discussion is about how the behavior differs from Rollup, but I'm not too familiar with Rollup's behavior. If the proposal is just aligning with Node's semantics, that would look like this for esbuild I think:

  • Exports from CommonJS objects are no longer live (they currently are in esbuild)
  • The Babel __esModule property is no longer respected (it is currently respected in esbuild)

But wouldn't that break compatibility with the existing package ecosystem? I'm trying to make esbuild compatible with the ecosystem so it has a high chance of "just working" with existing packages, as long as it still follows ECMAScript module semantics (e.g. module namespace objects must not be callable). If a package works with Webpack, I'm inclined to also have it work with esbuild, even if it doesn't work with Node. The package may not even be intended for consumption with Node so enforcing strict compatibility with how Node does things even if it breaks stuff seems like it makes unnecessary hassle for users of bundlers.

I could see having esbuild do something like the union of the two approaches. If __esModule is present you would both have a default export that points to module.exports and also have named exports for every property on module.exports at the moment in time when the initial import is complete. I think that would have compatibility with both Webpack and Node. Do you have any thoughts about that?

To fully specify the behavior you'd want to describe what is done with non-enumerable properties and properties on the prototype of module.exports. People expect both of those to be present on import module namespace objects because of legacy transpilers that incorrectly convert import statements to bare require calls.

@guybedford
Copy link
Contributor Author

guybedford commented Nov 15, 2020

It's very unfortunate that the situation with default is such a mess.

Interestingly it was actually created and specified to specifically support module.exports for Node.js compat, yet through inadequate planning and coordination we find ourselves here yes.

If __esModule is present you would both have a default export that points to module.exports and also have named exports for every property on module.exports at the moment in time when the initial import is complete. I think that would have compatibility with both Webpack and Node. Do you have any thoughts about that?

This is exactly what Node.js has now implemented with named exports extraction as described in https://nodejs.org/dist/latest-v15.x/docs/api/esm.html#esm_commonjs_namespaces. Node.js does this via a static analysis because it cannot execute CJS modules before the ES module execution phase and in the ES module specification named exports must be known at linking time, before execution.

As of the next 12.x release this month this named exports support will be fully backported and be the stable legacy target support base by early next yea. So it is fine for bundlers to extract named exports, and for modules without __esModule, but as per the documentation there yes they should not be live bindings and should be plucked while also offering the module.exports object as the default. Bundlers can even support more named exports than Node.js, as the compat subset is the really important aspect here.

But the compat subset breaks down on the default export when there is also an __esModule export. That's the specific problem being discussed in that thread where the alignment question comes in as it's an either or situation where compat thrashing between tools and environments may be the unfortunate situation.

@evanw
Copy link
Owner

evanw commented Nov 15, 2020

Here's a quick compatibility matrix I just generated to get a better idea of the situation:

  • Importing module.exports without the __esModule property:

    Node 15.2 Webpack 5.4 Rollup 2.33 Parcel 1.12 esbuild 0.8
    Normal property 🚫 🚫
    Non-enumerable property 🚫 😐 😐 🚫
    Property from prototype 🚫 😐 🚫
    __esModule property present 🚫 😐 🚫 🚫
    default export present
  • Importing module.exports with the __esModule property set to true:

    Node 15.2 Webpack 5.4 Rollup 2.33 Parcel 1.12 esbuild 0.8
    Normal property 🚫
    Non-enumerable property 🚫 😐 😐
    Property from prototype 🚫 😐
    __esModule property present 🚫 😐
    default export present 🚫 🚫

Here's what each icon means:

✅ = Always imported (never undefined)
😐 = Sometimes imported (either undefined or not depending on the import syntax used)
🚫 = Never imported (always undefined)

The normal, non-enumerable, and prototype properties were all set to true:

let o = Object.create({ inherited: true })
Object.defineProperty(o, '__esModule', {value: true})
o.normal = true
Object.defineProperty(o, 'nonEnumerable', {value: true})
module.exports = o

The test is a .mjs file in a package with "type": "module" that imports a .js file (the code above) in a package with "type": "commonjs".

It looks like esbuild should be generating a default import if __esModule is present to match Webpack, Node, and Rollup. I can make that change.

For the discussion, it seems like tools seeking to maximize compatibility will end up just doing the union of what other tools do. So that's where my preference would be and I'm imagining that's where things will end up.

@guybedford
Copy link
Contributor Author

guybedford commented Nov 15, 2020

@evanw I'd suggest using a Node.js statically analyzable test case as well for the Node.js tests:

exports.name = 'value';

I'd also suggest checking the shape of import * as m from 'cjs' and whether it has a default as module.exports with a named exports shell, or whether it has just the exports.

It looks like esbuild should be generating a default import if __esModule is present to match Webpack, Node, and Rollup. I can make that change.

The default should always be module.exports, regardless of __esModule. The question is just what to do when it clashes with a default export with __esModule.

For the discussion, it seems like tools seeking to maximize compatibility will end up just doing the union of what other tools do. So that's where my preference would be and I'm imagining that's where things will end up.

Agreed, but the thrashing issue I'm talking about is when there is mutual exclusivity in the solution space with the case I describe above.

@guybedford
Copy link
Contributor Author

And I will still be pushing very hard to get RollupJS to change its default behaviour here for the edge case to match Node.js as well, as I think very slightly worse compat now is better than a future of bad compat. The greedy algorithm of software evolution that usually doesn't do too badly breaking down exactly why I'm having this discussion with you.

@evanw
Copy link
Owner

evanw commented Nov 15, 2020

The default should always be module.exports, regardless of __esModule. The question is just what to do when it clashes with a default export with __esModule.

Ah, I finally understand. That is tricky. Now that I think about it, I'm pretty sure that's why I didn't make esbuild generate a default export in this case.

Here's a simple test file (statically analyzable this time):

Object.defineProperty(module.exports, '__esModule', { value: true })
module.exports.default = 123

This is how the different environments behave on this one as far as the value of the import default:

Node 15.2 Webpack 5.4 Rollup 2.33 Parcel 1.12 esbuild 0.8
{default: 123}
123

Node and Webpack are the majority of the ecosystem so if that's the case, I think that should be the behavior to converge to.

@evanw
Copy link
Owner

evanw commented Nov 15, 2020

Actually, after thinking about it more I'm not totally sure. Doing that means that converting ESM to CJS can no longer be done correctly, which seems like a mistake.

In particular, that would mess up esbuild's automatic ESM-to-CJS transform that happens in certain scenarios. For example, if an ESM module has a direct call to eval in it, esbuild will convert the ESM to CJS during bundling to ensure that the names in the top-level scope can be preserved (since eval may reference them) without causing collisions with names from other modules in the bundle due to scope hoisting. This works fine now but would break if this change was implemented.

I could always work around this by making esbuild's automatic ESM-to-CJS transform special somehow and excluding files transformed by it from this rule. It'd be great to not have to do that since doing that is even more complexity but I'm not sure what the alternative is.

@guybedford
Copy link
Contributor Author

eval is a really interesting case, and I don't believe RollupJS makes any allowance for it currently. In theory under a strict optimization, eval should never merge its bindings to ensure they are always the same scope names. In reality it really depends on the patterns that dominate what rules make sense. Even the conversion might break down on cases like imported namespaces or live bindings or this access (exports in CJS, undefined in ESM), within the eval. Out of interest, what sort of use cases did eval support come up for?

@evanw
Copy link
Owner

evanw commented Nov 16, 2020

Out of interest, what sort of use cases did eval support come up for?

It's mainly just me trying to implement a correct bundler and minifier. For example, I've been looking at test cases from the UglifyJS and Terser projects and there are lots of test cases about direct eval.

Of the real-world uses of direct eval I've seen:

  • A lot of the use of it at my workplace is for code that is run during tests, although that is also bundled and needs to work correctly
  • Some appear to be only trying to access globals such as Object, in which case of course global eval is sufficient (and better even)
  • Some appear to be accidental uses by people who don't know what direct eval is or why it's bad to publish code that uses it
  • Some of it is deliberate but meant to access identifiers that aren't declared within that file but that also aren't declared as globals such as eval('require')
  • Some of it is deliberate but is intended to access local variables declared inside a function scope (we have a test helper that uses this to set up some data-driven tests)
  • None of it attempts to access top-level variables in the module

There are other cases where esbuild does ESM-to-CJS in addition to eval:

  • I have decided to have esbuild support both CJS and ESM within the same module so users don't have to deal with getting the script vs. module goal wrong. If you use CJS features (exports or module or a top-level return) any ESM syntax will be converted to CJS. This works pretty seamlessly at the moment since CJS bindings are live when imported into ESM and the default export from the original ESM before the conversion to CJS is respected.

  • Another case is when an ESM module does a re-export of a CJS module using export * from. Since exports in CJS are dynamic at run-time, the re-exports are computed at run-time too. That requires the re-exporting module to also be converted from ESM to CJS.

@sokra
Copy link

sokra commented Nov 16, 2020

Yeah, another interop discussion...

Note that webpack has two different modes. One that's compatible with the ecosystem and one that's compatible with Node.js.

Node.js compat mode is used when import originates from a .mjs file (or type: module). Otherwise the normal mode is used.

That are the main differences:

  • __esModule + default export is handled correctly in normal mode and Node.js compatible in Node.js compat mode (default is module.exports).
  • Normal mode auto-detects ESM vs CommonJs, while Node.js mode only supported ESM.
  • import * as x behaves like x = require(...) in normal mode (Babel compat), while it correctly creates a namespace object in Node.js mode.

And btw. commonjs imports are always live in webpack. Not having live-bindings would break modules that are transpiled from ESM to CJS, so it doesn't really make sense to me why it is this way in Node.js...

@evanw
Copy link
Owner

evanw commented Nov 16, 2020

Note that webpack has two different modes. One that's compatible with the ecosystem and one that's compatible with Node.js.

Node.js compat mode is used when import originates from a .mjs file (or type: module). Otherwise the normal mode is used.

Thanks for pointing this out. It didn't occur to me that this could be the case. This is interesting. Perhaps this could be the approach esbuild ends up taking too.

And btw. commonjs imports are always live in webpack. Not having live-bindings would break modules that are transpiled from ESM to CJS, so it doesn't really make sense to me why it is this way in Node.js...

Good to know. I plan on keeping them live in esbuild too, since that would obviously break ESM code that has been converted to CJS (including code that esbuild auto-converts itself which needs to continue to work correctly, as described above).

@sokra
Copy link

sokra commented Nov 17, 2020

I created a larger test repo to analyse the interop behavior of different tools:

https://sokra.github.io/interop-test/

@guybedford Have fun analysing that...

@evanw One interesting case is module.exports = null; which leads to a runtime error in esbuild.

btw. parcel doesn't run on windows, but I created the test case anyway. Maybe someone can run it on a Mac and PR the results.

@evanw
Copy link
Owner

evanw commented Nov 18, 2020

I created a larger test repo to analyse the interop behavior of different tools:

https://sokra.github.io/interop-test/

This is a fantastic resource. Thanks for creating this.

@evanw One interesting case is module.exports = null; which leads to a runtime error in esbuild.

I was not aware of that case. Thanks for the heads up. Will fix.

evanw added a commit that referenced this issue Nov 18, 2020
@sokra
Copy link

sokra commented Nov 18, 2020

I was not aware of that case. Thanks for the heads up. Will fix.

Works great, updated: https://sokra.github.io/interop-test/#esbuild

Another runtime crash happens when module.exports = Promise.resolve(42) is used via import(). The then property is copied into the namespace object and import() tries to call it, but with an incorrect this object.

@sokra
Copy link

sokra commented Nov 18, 2020

The good thing is I can now link you a summary how other tools behave in this case: https://sokra.github.io/interop-test/#single-promise-object-export

PS: I had the same problem in webpack 5 and have chosen that the Promise should be resolved and the result should be returned from import(). Similar to how import()ing export function then(fn) { ... } behaves.

@evanw
Copy link
Owner

evanw commented Mar 4, 2021

This way esbuild handles this is changing slightly in the next release. I tried going all-in on the node behavior but it broke compatibility so I'm rolling that back to being mostly still Babel-compatible. This release fixes the case where __esModule is present but there is no default export. Previously the default import was undefined but in this release the default import is now the module's exports.

Given:

import defaultValue from './some-cjs-file'

Old behavior:

defaultValue is module.exports unless !!module.exports.__esModule, in which case defaultValue is module.exports.default

New behavior:

defaultValue is module.exports unless !!module.exports.__esModule and "default" in module.exports, in which case defaultValue is module.exports.default

@o-alexandrov
Copy link

o-alexandrov commented Jul 4, 2021

Is the following problem when transpiling with esbuild relevant to this issue?
A module with several named exports and without a default export:

  • result in the following value when imported all as import * as privacySettings from "./name-of-module"

Screen Shot 2021-07-03 at 10 58 44 PM

This generated default export value by esbuild is different from webpack & rollup processing.
webpack v4-5 & rollup v2 don't generate an undeclared default export value

@evanw
Copy link
Owner

evanw commented Jul 4, 2021

It's unclear what's going on because you didn't post a complete code sample but yes, that sounds like the same thing that's being discussed here. Importing a CommonJS file using import * as in node always generates a default property with the value of module.exports and esbuild was changed to try to match node, so I assume that's what your code is doing.

If so, you should be able to run import * as privacySettings from "./name-of-module.js" natively in node without bundling and get the same result as esbuild gives here. It's true that other bundlers may do something different from what node does but esbuild does this because it's trying to match node.

Not including a default property in this case breaks node-based libraries that expect it to work since node provides default. I think #909 was an example of such breakage. And there are other libraries that break when default is included since they were designed for Babel's cross-compilation approach that omits default (which was the primary implementation before node introduced its different one later on). Since some things break with default and other things break without default, there's no general way to prevent problems in all cases.

TL;DR: The situation is a mess and you should try to avoid using or relying on default properties.

@o-alexandrov
Copy link

@evanw please find a reproduction of this bug here:

The complete code sample there is 2 files:

// named-exports.js
export const a = "a";
export const b = "b";
// index.js
import * as namedExports from "./named-exports";

console.log(namedExports);
console.log(namedExports.default);

There are two npm scripts: nodejs & esbuild.
When you run npm run nodejs, you'd see the following output in the console:

[Module] { a: 'a', b: 'b' }     // namedExports
undefined                           // namedExports.default

whereas when you run npm run esbuild, the CLI output is:

  • note there's the unexpected default value
{ default: { a: [Getter], b: [Getter] }, a: [Getter], b: [Getter] }
{ a: [Getter], b: [Getter] }

As you could see above, esbuild provides a different output than plain Node.js either with esm to transpile JS, or by running against .mjs files.


On a side note, your advice is irrelevant as I'm not relying on default property to exist:

TL;DR: The situation is a mess and you should try to avoid using or relying on default properties.

@mnpenner
Copy link

mnpenner commented Jan 22, 2022

So what's the solution here? How do I output a Typescript lib to be compatible with node?

i.e. given source:

export default function jsSerialize(object: any, options?: Partial<Options>): string {
    return "whatever";
}

If I build my lib with:

npx esbuild --bundle src/index.ts --outdir=dist --platform=node --target=node14

And then try to use the lib:

❯ node
Welcome to Node.js v16.9.1.
Type ".help" for more information.
> js = require('./dist')
{ default: [Getter] }
> js('hello')
Uncaught TypeError: js is not a function

i.e. it's outputting the default property, which still isn't compatible with vanilla node 16.x. I don't want consumers to have to recompile my lib or to have to use special flags or .default('hello') for my lib to work.

@hyrious
Copy link

hyrious commented Jan 23, 2022

@mnpenner you can write a wrapper to edit the exports object:

// cjs-wrapper.js
module.exports = require("./dist/index.js").default

// package.json
"main": "cjs-wrapper.js"

@b-smets
Copy link

b-smets commented Feb 3, 2022

We ran into this issue as well with the dom-serializer module.

I was able to work around it by adding the following wrapper:

module.exports = require('dom-serializer').default;

I then added a custom plugin to resolve this wrapper when importing the dom-serializer default export this way:

module.exports = {
  name: 'fix-dom-serializer',
  setup(build) {
    build.onResolve({ filter: /^dom-serializer$/ }, ({ kind }) => {
      if (kind === 'import-statement') {
        return { path: require.resolve('./dom-serializer-wrapper') }
      }
    })
  },
}

By checking the kind field we ensure this is only done for imports in ESModules, leaving CommonJS modules untouched.

The importing ESModule looks like:

import serialize from 'dom-serializer;
// ...
serialize(element);

The bundled code of the module using dom-serializer then looks like:

import_dom_serializer = __toESM(require_dom_serializer_wrapper(), 1);
//...
(0, import_dom_serializer.default)(element);

I'm not sure if this is the recommended workaround but it seems to work. It can probably be avoided if dom-serializer would have an esm export.

@evanw
Copy link
Owner

evanw commented Feb 18, 2022

I wrote up some documentation about this here: https://esbuild.github.io/content-types/#default-interop

@clshortfuse
Copy link

clshortfuse commented Mar 12, 2022

Is using a wrapper still required? I feel like it would be nice to have some sort of flag somewhere. I can't think of a package that intends users to do require('packagename).default. Would would be the consequences of detecting if there's only one export and that export is default, then for esbuild to do module.exports = foo instead of module.exports = { default: foo }.

My usage is coding in ESM only and use esbuild to build CJS for backwards compatibility. I would like to have the same syntax for both import and require usage. eg: import package from 'packageName' and const package = require('packageName'). Instead it's forcing import package from 'packageName' and const package = require('packageName').default.

I also have multiple exports, so it wouldn't be a single wrapper, but several:

"exports": {
    "./lib/*": "./lib/*",
    ".": {
      "require": "./exports/index.cjs",
      "import": "./exports/index.js"
    },
    "./stream": {
      "require": "./exports/stream.cjs",
      "import": "./exports/stream.js"
    }
  },

Edit: rollup allows us to do this and does this by default with the following warning:

(!) Entry module "exports/index.js" is implicitly using "default" export mode, which means for CommonJS output that its default export is assigned to "module.exports". For many tools, such CommonJS output will not be interchangeable with the original ES module. If this is intended, explicitly set "output.exports" to either "auto" or "default", otherwise you might want to consider changing the signature of "exports/index.js" to use named exports only.

This is exactly what I would want (intended) and I would like if we can do the same with esbuild. :)

@titanism
Copy link

@mnpenner you can write a wrapper to edit the exports object:

// cjs-wrapper.js
module.exports = require("./dist/index.js").default

// package.json
"main": "cjs-wrapper.js"

Thanks for sharing this quick fix - we've implemented in a PR here to an ESM project with esbuild at expressjs/multer#1161.

@titanism
Copy link

Having a flag would be super nice though to do this automatically! Agreed @clshortfuse!

jtoar added a commit to redwoodjs/redwood that referenced this issue Mar 7, 2024
This PR builds on the work started in
#10083. It does two things:

- ~builds the `@redwoodjs/vite` package with esbuild instead of babel
(this is optional but just did it to debug, but I'd imagine we'd want to
start doing this anyway)~
  - CI wasn't happy with this change so removed for now
- introduces a CJS wrapper to handle ESM export default interoperation
in Node

To frame this change at a high level, one of the ways I'm approaching
ESM is backwards from Redwood apps. If Redwood apps can be ESM (via
"type": "module" in package.jsons, updates to tsconfigs, adding file
extensions to relative imports, etc), then we may have an easier time
rolling out packages that can't be shipped as dual ESM/CJS exports.
(Vite is probably one of them. But since Vite evaluates the config file
itself, it may be ok.)

One of the framework-level bugs that shows up when you make an app ESM
is this error:

```
failed to load config from ~/redwood/redwood-app-esm/web/vite.config.mts
file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918- 
6f6a7f8efad05.mjs:7
  plugins: [redwood()]
            ^

TypeError: redwood is not a function
    at file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918- 
6f6a7f8efad05.mjs:7:13
```

The culprit is this import in `web/vite.config.ts`:

```ts
import redwood from '@redwoodjs/vite'
```

The problem is confusing, but basically... when you 1) transpile an ES
module into CJS (which is what we're doing for most of our packages) and
then 2) import that CJS module from a bona fide ES module, the default
export doesn't behave how you'd expect. Evan Wallace has a great write
up of it
[here](https://esbuild.github.io/content-types/#default-interop). (For
Node's official docs, see
https://nodejs.org/docs/latest/api/esm.html#commonjs-namespaces.)

As a potential fix, @Tobbe pointed me to this Vite plugin:
https://github.com/cyco130/vite-plugin-cjs-interop. I tried adding it
but the issue is that this **is** the Vite config file. 😅

But I thought I may be able to add the plugin here when we call
`createServer`:


https://github.com/redwoodjs/redwood/blob/e9ecbb07da216210c59a1e499816f31c025fe81d/packages/vite/bins/rw-vite-dev.mjs#L28-L37

But it still wasn't working. I stepped through the source a bit and it
doesn't seem like there's room for configuring how Vite loads the config
file. The two main functions were these, `loadConfigFromBundledFile` and
`loadConfigFromFile`:
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L1186
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L962

`bundleConfigFile` has plugins configured, but they're hardcoded.

But luckily I don't think it's necessary... based on this esbuild
thread, it seems like we can get away with a small wrapper. See
evanw/esbuild#532. When we actually make this
an ES module (which will happen pretty soon I think), this will go away.
But for the time being, this is a CJS module.
ahaywood pushed a commit to redwoodjs/redwood that referenced this issue Mar 8, 2024
This PR builds on the work started in
#10083. It does two things:

- ~builds the `@redwoodjs/vite` package with esbuild instead of babel
(this is optional but just did it to debug, but I'd imagine we'd want to
start doing this anyway)~
  - CI wasn't happy with this change so removed for now
- introduces a CJS wrapper to handle ESM export default interoperation
in Node

To frame this change at a high level, one of the ways I'm approaching
ESM is backwards from Redwood apps. If Redwood apps can be ESM (via
"type": "module" in package.jsons, updates to tsconfigs, adding file
extensions to relative imports, etc), then we may have an easier time
rolling out packages that can't be shipped as dual ESM/CJS exports.
(Vite is probably one of them. But since Vite evaluates the config file
itself, it may be ok.)

One of the framework-level bugs that shows up when you make an app ESM
is this error:

```
failed to load config from ~/redwood/redwood-app-esm/web/vite.config.mts
file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918-
6f6a7f8efad05.mjs:7
  plugins: [redwood()]
            ^

TypeError: redwood is not a function
    at file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918-
6f6a7f8efad05.mjs:7:13
```

The culprit is this import in `web/vite.config.ts`:

```ts
import redwood from '@redwoodjs/vite'
```

The problem is confusing, but basically... when you 1) transpile an ES
module into CJS (which is what we're doing for most of our packages) and
then 2) import that CJS module from a bona fide ES module, the default
export doesn't behave how you'd expect. Evan Wallace has a great write
up of it
[here](https://esbuild.github.io/content-types/#default-interop). (For
Node's official docs, see
https://nodejs.org/docs/latest/api/esm.html#commonjs-namespaces.)

As a potential fix, @Tobbe pointed me to this Vite plugin:
https://github.com/cyco130/vite-plugin-cjs-interop. I tried adding it
but the issue is that this **is** the Vite config file. 😅

But I thought I may be able to add the plugin here when we call
`createServer`:

https://github.com/redwoodjs/redwood/blob/e9ecbb07da216210c59a1e499816f31c025fe81d/packages/vite/bins/rw-vite-dev.mjs#L28-L37

But it still wasn't working. I stepped through the source a bit and it
doesn't seem like there's room for configuring how Vite loads the config
file. The two main functions were these, `loadConfigFromBundledFile` and
`loadConfigFromFile`:
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L1186
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L962

`bundleConfigFile` has plugins configured, but they're hardcoded.

But luckily I don't think it's necessary... based on this esbuild
thread, it seems like we can get away with a small wrapper. See
evanw/esbuild#532. When we actually make this
an ES module (which will happen pretty soon I think), this will go away.
But for the time being, this is a CJS module.
jtoar added a commit to redwoodjs/redwood that referenced this issue Mar 8, 2024
This PR builds on the work started in
#10083. It does two things:

- ~builds the `@redwoodjs/vite` package with esbuild instead of babel
(this is optional but just did it to debug, but I'd imagine we'd want to
start doing this anyway)~
  - CI wasn't happy with this change so removed for now
- introduces a CJS wrapper to handle ESM export default interoperation
in Node

To frame this change at a high level, one of the ways I'm approaching
ESM is backwards from Redwood apps. If Redwood apps can be ESM (via
"type": "module" in package.jsons, updates to tsconfigs, adding file
extensions to relative imports, etc), then we may have an easier time
rolling out packages that can't be shipped as dual ESM/CJS exports.
(Vite is probably one of them. But since Vite evaluates the config file
itself, it may be ok.)

One of the framework-level bugs that shows up when you make an app ESM
is this error:

```
failed to load config from ~/redwood/redwood-app-esm/web/vite.config.mts
file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918-
6f6a7f8efad05.mjs:7
  plugins: [redwood()]
            ^

TypeError: redwood is not a function
    at file:///~/redwood/redwood-app-esm/web/vite.config.mts.timestamp-1709251612918-
6f6a7f8efad05.mjs:7:13
```

The culprit is this import in `web/vite.config.ts`:

```ts
import redwood from '@redwoodjs/vite'
```

The problem is confusing, but basically... when you 1) transpile an ES
module into CJS (which is what we're doing for most of our packages) and
then 2) import that CJS module from a bona fide ES module, the default
export doesn't behave how you'd expect. Evan Wallace has a great write
up of it
[here](https://esbuild.github.io/content-types/#default-interop). (For
Node's official docs, see
https://nodejs.org/docs/latest/api/esm.html#commonjs-namespaces.)

As a potential fix, @Tobbe pointed me to this Vite plugin:
https://github.com/cyco130/vite-plugin-cjs-interop. I tried adding it
but the issue is that this **is** the Vite config file. 😅

But I thought I may be able to add the plugin here when we call
`createServer`:

https://github.com/redwoodjs/redwood/blob/e9ecbb07da216210c59a1e499816f31c025fe81d/packages/vite/bins/rw-vite-dev.mjs#L28-L37

But it still wasn't working. I stepped through the source a bit and it
doesn't seem like there's room for configuring how Vite loads the config
file. The two main functions were these, `loadConfigFromBundledFile` and
`loadConfigFromFile`:
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L1186
-
https://github.com/vitejs/vite/blob/e92abe58164682c2e468318c05023bfb4ecdfa02/packages/vite/src/node/config.ts#L962

`bundleConfigFile` has plugins configured, but they're hardcoded.

But luckily I don't think it's necessary... based on this esbuild
thread, it seems like we can get away with a small wrapper. See
evanw/esbuild#532. When we actually make this
an ES module (which will happen pretty soon I think), this will go away.
But for the time being, this is a CJS module.
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 a pull request may close this issue.

9 participants