tslib icon indicating copy to clipboard operation
tslib copied to clipboard

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

Open rbuckton opened this issue 2 years ago • 19 comments

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

rbuckton avatar Feb 08 '22 18:02 rbuckton

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

DanielRosenwasser avatar Feb 08 '22 19:02 DanielRosenwasser

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.

rbuckton avatar Feb 09 '22 00:02 rbuckton

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 avatar Feb 09 '22 00:02 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 avatar Feb 09 '22 00:02 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.

weswigham avatar Feb 09 '22 00:02 weswigham

What entrypoint should vite be using then?

rbuckton avatar Feb 09 '22 02:02 rbuckton

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.

weswigham avatar Feb 09 '22 03:02 weswigham

We should probably add a test for vite as well.

rbuckton avatar Feb 10 '22 19:02 rbuckton

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.

johncrim avatar Feb 27 '22 20:02 johncrim

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.

thescientist13 avatar Mar 01 '22 15:03 thescientist13

@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"

johncrim avatar Mar 04 '22 00:03 johncrim

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. 💯

thescientist13 avatar Mar 06 '22 17:03 thescientist13

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/[email protected][email protected]/node_modules/jest-runtime/build/index.js:669:5)
      at TestScheduler.scheduleTests (node_modules/.pnpm/@[email protected][email protected]/node_modules/@jest/core/build/TestScheduler.js:333:13)
      at runJest (node_modules/.pnpm/@[email protected][email protected]/node_modules/@jest/core/build/runJest.js:387:19)
      at _run10000 (node_modules/.pnpm/@[email protected][email protected]/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/[email protected]/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.

wmertens avatar Apr 07 '22 23:04 wmertens

@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.

johncrim avatar Apr 08 '22 02:04 johncrim

@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 avatar Apr 08 '22 05:04 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

wmertens avatar Apr 09 '22 20:04 wmertens

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: https://github.com/microsoft/tslib/issues/143#issuecomment-1035472917

benmccann avatar Oct 14 '22 19:10 benmccann

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.

justinfagnani avatar Apr 26 '23 22:04 justinfagnani

@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.

andrewbranch avatar May 01 '23 18:05 andrewbranch