5.0.0-rc.3 fails with tslib

[philcockfield]

Bug report

What is the current behavior?

Running [email protected] fails when importing rxjs (which uses tslib):

---

If the current behavior is a bug, please provide the steps to reproduce.

Details of this error are reproducible in https://github.com/philcockfield/webpack5-tslib-error
The demo repo is a minimal webpack configuration using ts-loader that imports rxjs. It also behaves this way with vanilla JS and babel loaders.

---

What is the expected behavior?
The page loads with no errors. This demo repo works fine with [email protected] and then cleanly fails by flipping the version up to 5.0.0-rc.3

---

Other relevant information:
webpack version: 5.0.0-rc.3, 5.0.0-rc.4
Node.js version: 12.16.3
Operating System: MacOS (10.15.7)
Additional tools: -

[aariacarterweir]

Seeing this too with @sentry/browser

[sokra]

@orta This is caused by microsoft/tslib#127

tslib/modules/index.js contains a import tslib from '../tslib.js';
but tslib/tslib.js doesn't contain a default export. It only contains named exports.
Becauses of this tslib is undefined in tslib/modules/index.js.
webpack 4 is unaffected because it doesn't support the exports field.
Node.js is unaffected because it doesn't support the __esModule flag.
But in combination they lead to this error.

Unrelated from this adding an exports field to a package.json is most often a breaking change, because it breaks e. g. import "tslib/tslib.js". I would recommend to revert the backport.

[weswigham]

@sokra the exports pattern in use has a directory reexport, so import "tslib/tslib.js" should continue to function as it used to.

The issue is solely because webpack and node disagree on what shape importing a cjs module should take. In node, when an es module imports a cjs module, the cjs module is made available, in full, as the default value of the import. Some statically determinable names may also be made available as named exports, as of a recent patch, but tslib's exports aren't statically determinable (because of its UMD wrapper pattern), so in node it won't have any named exports. Meanwhile in webpack, only named exports get made.... these two views of the module system are mutually incompatible. Either node or webpack would need to change:

• If node could get named exports without static analysis and use the values available at runtime, then we could use named exports everywhere (as you suggest) and it'd be fine. (In fact, if node did this, we wouldn't need our exports redirection to support node's esm at all!)
• If webpack made the cjs module available as the default, as node does, then we could use the default as we currently do, and it'd be fine.

One of the two needs to change, though, there's nothing we could do that isn't a major break in some other way that fixes it.

[guybedford]

I thought webpack did make the default available when importing CommonJS modules? Is that really the root cause here?

[weswigham]

Yeah, it would seem to be. Both babel and TS (with esModuleInterop) make the module available as the default when importing cjs modules; it would seem that webpack does not.

[guybedford]

I specificaly tested this and it seemed to always include default. Is it perhaps because there is an __esModule export which is blocking the normal default set?

[guybedford]

Further, if tslib isn't being imported but it is being require'd the "import" path shouldn't be applying in exports in the first place at all. So I'm confused why it's even using the module verison to begin with.

[alexander-akait]

@guybedford exports works for import and require

[guybedford]

"import" should not be matched when using a require() expression, it is very clearly defined not to mean that.

[alexander-akait]

And we don't do it

[guybedford]

rxjs is CommonJS loading tslib - so it shouldn't be hitting the "import" path in require('tslib'), but it sounds like it is?

[guybedford]

I see rxjs has a modules path. So it's just the issue in #11613 (comment) then. Ok glad to hear "imports" doesn't apply unnecessarily and thanks for clarifying @evilebottnawi.

It should be possible to support default for __esModule CommonJS. This leaves only one conflict case where a module has both __esModule and default. On this edge case I understand Node.js and bundlers likely will have to just remain in conflict. But we can solve it right up to that edge at least, which is where all the work on both sides finally gets us, that one small gap :)

[alexander-akait]

@guybedford don't worry we will found better way, you can see blocking on the right side above

[weswigham]

Although, as an aside, tslib also specifies a module field in the package.json - shouldn't that be higher priority for webpack than exports conditions? (Because of things like this?)

[sokra]

Yes, the behavior for the default export with __esModule flag differs between Node.js and other tools like webpack, babel, typescript, rollup, parcel.

Here is a simplified example:

// a.js
exports.__esModule = true;
exports.named = 42;

// b.js
exports.__esModule = true;
exports.named = 42;
exports.default = 123;

import a from "./a.js"
import b from "./a.js"

console.log(a, b);

Node.js will log { named: 42 } { named: 42, default: 123 } while webpack will log undefined 123.
Node.js basically ignores __esModule for the default export, which is not something I think is the intention of the user.

We could do a "default" in module check everytime somebody accesses the default export, but I'm not sure if that worth the performance cost for this edge case. Additionally you would need to convince all tools, since I'm not willing to give up the current status of agreement since it was a long way to get all tools to the same behavior.

---

As workaround add exports.default = exports; to tslib/tslib.js, or import * as tslibModule from "../tslib"; const tslib = tslibModule.default || tslibModule; to tslib/modules/index.js

[weswigham]

@sokra but again, shouldn't webpack be using the module package.json field?

[sokra]

shouldn't webpack be using the module package.json field?

no exports has higher priority. module is the fallback for "old" tools.

Note that there is also a "module" condition in exports, but that isn't the right tool here.

[weswigham]

no exports has higher priority. module is the fallback for "old" tools.

If you're not faithfully emulating what node does for the import field (and you're not), I'm not sure that should be the case. The module field is expressly for webpack.

[sokra]

the exports pattern in use has a directory reexport, so import "tslib/tslib.js" should continue to function as it used to.

Oh sorry missed that. Should be fine then

[guybedford]

We could do a "default" in module check everytime somebody accesses the default export, but I'm not sure if that worth the performance cost for this edge case. Additionally you would need to convince all tools, since I'm not willing to give up the current status of agreement since it was a long way to get all tools to the same behavior.

RollupJS supports this I believe. I feel quite strongly the default invariant should be maintained whenever possible.

[sokra]

If you're not faithfully emulating what node does for the import field (and you're not), I'm not sure that should be the case. The module field is expressly for webpack.

The exports field is not really relevant for this problem. If you point module to tslib/modules/index.js it would have the same issue. Or if you importing import tslib from "tslib/modules/index.js" directly.
If something you could claim that "type": "module" should make webpack behave like Node.js.

[guybedford]

I wouldn't tie "type": "module" to any behaviour. "exports" should behave the same regardless. But since rxjs is running as an ES module it does seem Webpack is following the "import" path correctly which was my misunderstanding too.

[sokra]

RollupJS supports this I believe.

Yes, but that's deprecated. It also checks only "default" in module and not module.__esModule.

I feel quite strongly the default invariant should be maintained whenever possible.

I feel like __esModule should behave like the non-transpiled counterpart.

// a.js
exports.__esModule = true;
exports.named = 42;

// a.js (non-transpiled)
export const named = 42;

[weswigham]

We only have a condition directing to a wrapper module because node can't read the named exports we want without the assistance provided by the wrapper module - because webpack defers this work, and more importantly, performs esm substituions throughout the tree regardless of caller kind, we really should just be getting webpack to use our module entrypoint (while node continues to use this wrapper). Our exports redirects to a wrapper (rather than a copy) so that in node we still only load 1 copy of tslib, for both esm and cjs usages. In webpack, again, because of how the module field has historically worked, we were able to rely on their being only a single (esm) copy in the compilation.

Is there a way to get webpack to prioritize a module-like entrypoint over the exports condition?

[guybedford]

I feel like __esModule should behave like the non-transpiled counterpart.

Because Node.js has no treatment for __esModule. To ignore the default, is to break all interop of all __esModule cases between Node.js and Webpack.

For Node.js it was important to always maintain the invariant that import x from 'cjs' will always be the CommonJS module.exports, even for __esModule, because from Node.js perspective it is still just CommonJS. I understand Webpack wants to lift these, but in the case where they don't have a default export a double hole will smooth this case.

This is why I say this is one of those points to meet in the middle.

[sokra]

Is there a way to get webpack to prioritize a module-like entrypoint over the exports condition?

No, but there is a "webpack" condition for exports you can use. I would not recommend that, because it will still break the other tools (even while not currently, because they don't support "exports" yet).

[weswigham]

No, but there is a "webpack" condition for exports you can use. I would not recommend that, because it will still break the other tools (even while not currently, because they don't support "exports" yet).

So long as webpack differs from node in its handling (which is justified, imo, it's been around doing this longer~), we'll need to specify that condition~

[guybedford]

It's clearly worse if we have to individually configure every tool. I would strongly recommend alignment on the semantic edge case causing this issue. SystemJS has supported both __esModule and default patterns together since 2017.

[weswigham]

@sokra "node" and "default" top level exports members seem like an abuse of the format - in node, those would map to tslib/node and tslib/default import paths, which seems.... wrong (which is just me saying I don't like nested conditions and prefer a flat list).

[sokra]

I would strongly recommend alignment on the semantic edge case causing this issue.

I brought this up many times. Node.js always brought up technically problems with aligning with the correct __esModule semantics. Babel invented that like probably 5 years ago...

[guybedford]

@sokra I invented that pattern for Traceur in 2014.

[sokra]

microsoft/tslib#130 that definitely the better way compared to using the "webpack" condition.

[weswigham]

I am not maintaining "cross-compatible wrapper code", and no sane individual should be expected to do as such. I will just use the exports condition that makes webpack go back to unconditionally including the pure-es6 sources, as it did before we added the exports field for node.

[weswigham]

But while I do have a workaround for this case, I'll be clear here: This is undoubtedly a real issue that is going to continue to haunt both node and webpack for the foreseeable future (especially, as cjs and esm sources start to mix further). This is absolutely not specific to something odd that only tslib has done.

[sokra]

This is absolutely not specific to something odd that only tslib has done.

I think it's pretty rare, as hardly anyone writes code with __esModule manually. It's intended as target of ESM to CJS transpilation.

[weswigham]

For better or for worse, TS has supported when targeting cjs a export = expression, which, if used along the lines of

import mod = require("mod");
export = mod;

if "mod" has an __esModule marker, so, too, will the resulting module there (since it quite literally assigns the module object). Unfortunately for node, none of the exports will be statically determinable in the resulting cjs file, so to node, there will be no named exports, while to webpack, there will be no default, thanks to the marker.

So it's not quite limited to handwritten __esModule markers.

[guybedford]

This does affect any other transpiled package with an ESM wrapper (admittedly, where Node.js can't detect the exports).

[sokra]

Unfortunately for node, none of the exports will be statically determinable in the resulting cjs file, so to node, there will be no named exports, while to webpack, there will be no default, thanks to the marker.

I see the problem, but from webpack point of view that's the correct behavior. If the module mod has only named exports and no default export, and you reexporting the whole module with export = mod, then the resulting module has the same exports and that means it has no default export and only named exports. That these named exports are not accessible in Node.js (due to limitations of statically analyse-ablility) is a Node.js problem.

[sokra]

import mod = require("mod");
export = mod;

Even if you use this example when mod has only a default export (and __esModule), you would expect that the resulting module has the same default export. And not exporting { default: ..., __esModule: true } in Node.js.

[weswigham]

Yeah, that's where I thought this was going - node saying it's a webpack problem, and webpack saying it's a node problem. Which is why I'd simply like to override the sources webpack reads, since the module field we used previously did exactly that (and it's better for tree shaking, besides).

[sokra]

If you really want to solve that with exports conditions you could use that:

    "type": "module",
    "exports": {
        ".": {
            "module": "./tslib.js",
            "import": "./modules/index.js",
            "default": "./tslib.js"
        },
        "./": "./"
    }

But I would recommend to change tslib.js to ESM in the next major and offer a transpiled tslib.cjs version in addition:

    "type": "module",
    "exports": {
        ".": {
            "module": "./tslib.js",
            "require": "./tslib.cjs",
            "default": "./tslib.js"
        },
        "./": "./"
    }

This doesn't lead to duplication in webpack. It only leads to duplication in Node.js, but that's not relelvant here.

The benefit of using an ESM version is that more optimizations are possible for ESM.

[weswigham]

Yeah, we'll be adding an extra condition shortly; we're just ginning up some tests.

But again, the differences in the interop story are a real issue - working around it by including an esm copy of the sources in webpack won't necessarily be an option for everyone else...

[sokra]

fixed in tslib

[philcockfield]

Nice! 🍰
Congrats on the 5.0.0 release.