Move helpers from tslib.es6.js to modules/index.js

[rbuckton]

This moves the helpers out of tslib.es6.js and into modules/index.js so that ES Module imports don't depend on the CommonJS/UMD definitions. In addition, tslib.es6.js now just re-exports modules/index.js so that we still only have to maintain two copies of the helpers (as opposed to adding a third).

This addresses an issue with Svelte/Vite where they don't seem to like the re-export of the CommonJS version of the helpers.

Fixes #143

[DanielRosenwasser]

Would this be a patch-change? What's the risk of doing that?

[rbuckton]

I think this is a patch-level change. If you're able to import tslib.es6.js then its highly likely that the import to modules/index.js would succeed. The surface area of tslib.es6.js and modules/index.js doesn't change.

[weswigham]

While this works because tslib is fairly stateless, I think it's a bad idea because it loads two copies of every helper function into any node program that ends up depending on both the cjs and esm versions of tslib. (Which is probably most of them.)

Since you can't reasonably make the cjs helpers pull in the esm ones, only the other way around (as currently written) makes sense.

tslib already gets loaded multiple times by various ts-using libraries specifying mutually incompatible versions; should we really be increasing the number of copies loaded essentially by default for... what seems like another tool's build issue? The tslib.es6.js version is already specified as the module entry point, so any bundler that is replacing modules all-up in the dependency graph will already only load the es6 version. I'm pretty sure the actual issue is on vite here; we're doing nothing wrong (from our perspective).

[weswigham]

The other option would be to use the node condition instead of import (or alongside), so vite falls back to a different entry point. (But seriously, why don't they use module if present? it's pretty much designated for their use-case)

[weswigham]

Yeah, the more I think about it, the more I'm convinced that we should just slam a node condition into the map that uses the existing import entry point, so vite can't think it's for non-node imports. Because it's distinctly only for node's esm.

[rbuckton]

What entrypoint should vite be using then?

[weswigham]

If it doesn't respect type: module (and if it can't load cjs, I can't imagine it does), we can point it at the existing tslib.es6.js using a non-node import condition.

[rbuckton]

We should probably add a test for vite as well.

[johncrim]

I opened #173 before seeing this thread. Would it be possible to rename tslib.es6.js to tslib.mjs (that would address the node resolution logic when resolving ES modules)? I've stepped through the node module resolution code a bunch of times (primarily while figuring out why certain modules, like tslib, aren't being resolved), and it doesn't seem to find the ./modules/index.js without the developer specifying an explicit mapping for tslib.

I certainly understand the need to not break existing code and not causing duplicate helpers in the codebase.

[thescientist13]

Would it be possible to rename tslib.es6.js to tslib.mjs (that would address the node resolution logic when resolving ES modules)?

I'm not sure renaming the file helps if the export map is specifically pointing to what should be an ESM compatible file. The issue as I understand and experience as Vite team also does is that in trying to follow the export map conditions as they are defined, and explicitly favoring the ESM entry, you will end up with a file that is a not ESM. The name doesn't really matter much here IMO but instead the export map entry could just be changed to point to an ESM file as would be expected given the export map definition in place.

[johncrim]

@thescientist13 : Node has documented their logic for determining whether a module is CJS or ESM. If you follow that (and I have, and have also stepped through it in a debugger), the file extension absolutely matters. .mjs is always ESM, and .cjs is always CJS. It's only .js files that are ambiguous whether they're CJS or ESM, and since tslib's package.json doesn't specify "type": "module" all the files with a .js extension are determined to be CJS.

The official node resolution doesn't understand either of:

  "module": "tslib.es6.js",
  "jsnext:main": "tslib.es6.js"

[thescientist13]

Hey @johncrim , good call and yes, I glossed over that part in focusing my comment more on the literal name, rather than also the extension change as well.

I was more coming from the perspective of a tool doing its own resolution as per the discussion in #173 and so in that case we would be following the spec of NodeJS resolution, but not relying on NodeJS for the resolution per se (aside from the location of the package on disk and then reading its export map / main / etc) from package.json. In my case I am using all this resolver information to build up an import map for the client side for browser based web development.

I think at the end of the day as long as ESM entry points only include ESM, everyone should be happy. 💯

[wmertens]

I'm having trouble with tslib under Jest with ESM. I'm getting

    SyntaxError: The requested module 'tslib' does not provide an export named '__decorate'

      at Runtime.linkAndEvaluateModule (node_modules/.pnpm/jest-runtime@27.0.6_supports-color@9.0.2/node_modules/jest-runtime/build/index.js:669:5)
      at TestScheduler.scheduleTests (node_modules/.pnpm/@jest+core@27.0.6_supports-color@9.0.2/node_modules/@jest/core/build/TestScheduler.js:333:13)
      at runJest (node_modules/.pnpm/@jest+core@27.0.6_supports-color@9.0.2/node_modules/@jest/core/build/runJest.js:387:19)
      at _run10000 (node_modules/.pnpm/@jest+core@27.0.6_supports-color@9.0.2/node_modules/@jest/core/build/cli/index.js:408:7)

and when I manually copy the tslib.es6.js file over the modules/index.js, it doesn't make a difference. Editing the package.json to point to modules/index.js has no effect either.

When I change the main to point to modules/index.js, the error becomes

Must use import to load ES Module: /home/wmertens/Projects/StratoSync/node_modules/.pnpm/tslib@2.3.1/node_modules/tslib/modules/index.js

So it seems that jest somehow imports tslib as cjs. I did do the ESM setup for Jest and the other tests work fine.

[johncrim]

@wmertens - That's correct that jest imports tslib as cjs, as I called out in #173. tslib currently has to be special-cased in node projects using ESM, thanks to the tslib layout and metadata.

It would be better to add your comment to an issue instead of a pull request, unless the comment is relevant to the pull request. The reason I commented here is that I wanted to make sure the reviewer was aware of other issues, like being compatible with node logic for determining CJS vs ESM.

[wmertens]

@johncrim ah ok thank you. I commented here because I wanted to point out that the PR would not help in my case (using jest but only using tslib indirectly). I'll take the conversation to #173

[wmertens]

I think the problem I'm encountering is that Jest is parsing the module and not detecting the exports. This PR doesn't solve the problem for me. Instead I manually created the exports and I have a workaround up at https://github.com/StratoKit/tslib

[benmccann]

I'm fairly involved in both Svelte and Vite and can answer questions or otherwise help coordinate.

But seriously, why don't they use module if present?

Vite does use module if present.

Vite will follow the Node spec and will ignore module if exports is present. However, Vite supports Node's conditional exports with the following default conditions: import, module, browser, default, and production/development.

We should probably add a test for vite as well.

It looks like that's been done, but changes to the test are needed to reproduce the issue: #143 (comment)

[weswigham]

There's no reason we need to do this, and this loads 2 copies of every helper in node proper, which is not what we wanted to do and defeats the point of having a modules/index.js almost entirely.

[justinfagnani]

The reason to do this would be to fix the bug where loading tslib from a loader that only supports standard JS modules and the standard Node export conditions causes an exception.

[andrewbranch]

@justinfagnani does the loader in question care about the lack of "type": "module" in the root package.json, or can it be pointed to the existing tslib.es6.js? I would think that something that only supports ESM wouldn’t care about that or file extensions, and just assume that any file it resolves to is going to be ESM.