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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Feb 8, 2022

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
Copy link
Member

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

@rbuckton
Copy link
Member Author

rbuckton commented Feb 9, 2022

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
Copy link
Member

weswigham commented Feb 9, 2022

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
Copy link
Member

weswigham commented Feb 9, 2022

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
Copy link
Member

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
Copy link
Member Author

rbuckton commented Feb 9, 2022

What entrypoint should vite be using then?

@weswigham
Copy link
Member

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
Copy link
Member Author

We should probably add a test for vite as well.

@johncrim
Copy link

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
Copy link

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
Copy link

johncrim commented Mar 4, 2022

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

@weswigham weswigham removed their request for review March 5, 2022 01:33
@thescientist13
Copy link

thescientist13 commented Mar 6, 2022

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
Copy link

wmertens commented Apr 7, 2022

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/[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.

@johncrim
Copy link

johncrim commented Apr 8, 2022

@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
Copy link

wmertens commented Apr 8, 2022

@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
Copy link

wmertens commented Apr 9, 2022

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
Copy link

benmccann commented Oct 14, 2022

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)

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tobiasdiez added a commit to tobiasdiez/nuxt-graphql-server that referenced this pull request Oct 26, 2022
tobiasdiez added a commit to tobiasdiez/nuxt-graphql-server that referenced this pull request Oct 26, 2022
@justinfagnani
Copy link

justinfagnani commented Apr 26, 2023

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
Copy link
Member

andrewbranch commented May 1, 2023

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

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 this pull request may close these issues.

modules/index.js should re-export tslib.es6.js instead of tslib.js
9 participants