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

Update esm loader hooks API #1457

Merged
merged 36 commits into from Oct 10, 2021
Merged

Conversation

jonaskello
Copy link
Contributor

@jonaskello jonaskello commented Sep 12, 2021

This is an attempt to fix #1372.

Link to docs for the new hooks API:
https://github.com/nodejs/node/blob/master/doc/api/esm.md#loaders

EDIT from @cspotcode:

@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #1457 (8b328d3) into main (4a0db31) will decrease coverage by 0.15%.
The diff coverage is 75.75%.

Impacted Files Coverage Δ
dist-raw/node-esm-default-get-format.js 71.42% <71.42%> (ø)
src/esm.ts 84.21% <73.68%> (-3.93%) ⬇️
src/index.ts 77.83% <91.66%> (+0.27%) ⬆️
dist-raw/node-package-json-reader.js 88.88% <0.00%> (+5.55%) ⬆️

@cspotcode
Copy link
Collaborator

Thanks for the PR. I'm swamped with work, so I'll do my best to review in a timely manner, but it might drag a bit. However, anyone who wants to use this PR before it is merged and published can install directly from git. npm's documentation explains how to install dependencies directly from a git branch instead npmjs.com. Our CI also uploads a package tarball as an artifact, which anyone can download and install locally.

Re the warnings from node about deprecated hooks: I think we can detect the node version and conditionally export only the hooks expected by that version of node. We can export undefined for the others. We do version detection in a few places in the codebase; here is one:

ts-node/src/index.ts

Lines 43 to 62 in aaf6052

/**
* Does this version of node obey the package.json "type" field
* and throw ERR_REQUIRE_ESM when attempting to require() an ESM modules.
*/
const engineSupportsPackageTypeField =
parseInt(process.versions.node.split('.')[0], 10) >= 12;
function versionGte(version: string, requirement: string) {
const [major, minor, patch, extra] = version
.split(/[\.-]/)
.map((s) => parseInt(s, 10));
const [reqMajor, reqMinor, reqPatch] = requirement
.split('.')
.map((s) => parseInt(s, 10));
return (
major > reqMajor ||
(major === reqMajor &&
(minor > reqMinor || (minor === reqMinor && patch >= reqPatch)))
);
}

@jonaskello jonaskello marked this pull request as draft September 12, 2021 21:40
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
@cspotcode
Copy link
Collaborator

Should we add node's nightly builds to our test matrix? We already test against nightly typescript.

@cspotcode
Copy link
Collaborator

Linking to actions/setup-node#80 re testing against node nightlies. If they don't have a good solution, we can roll our own I guess.

@jonaskello jonaskello marked this pull request as ready for review September 17, 2021 13:42
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
src/esm.ts Outdated Show resolved Hide resolved
@cspotcode
Copy link
Collaborator

I've added node nightly to the test matrix which is causing some failures. It also looks like stack traces are not showing correct line numbers for the errors. I don't know if this is a ts-node bug, a node nightly bug, or if those stack traces have never been correct.

@jonaskello
Copy link
Contributor Author

According to the docs it seems source return value for formats builtin and commonjs should be undefined. I added a check for this and I think it might fix the tests.

@jonaskello
Copy link
Contributor Author

Well, it seemed to fix all the tests but one :-).

@cspotcode
Copy link
Collaborator

That's great; do you know what's up with the remaining failing test?

@cspotcode
Copy link
Collaborator

I'm pretty sure the remaining test failures can be fixed by updating esm/transpile-only.mjs to export the new hooks. With that, this is probably ready to merge.

@jonaskello
Copy link
Contributor Author

jonaskello commented Oct 6, 2021

Thanks, I'll try to get some time to look into the last test failure.

FYI, I'm also working towards a more complete solution for the resolve hook in a new package called ts-resolve. It is still in an early stage but it would of course be interesting to know if something like this package would be considered useful for ts-node?

@cspotcode
Copy link
Collaborator

The fix was a one-line change; tests are passing now. 63c0618

it would of course be interesting to know if something like this package would be considered useful for ts-node?

Possibly, yeah. We want to do both CJS and ESM resolution so that projects "just work," so if we support project references for ESM it should also work with CJS. And since we already resolve the user's configuration, we'd want it to accept a pre-parsed config or set of multiple project-referenced configs, share FS cache, stuff like that. We'll also need to be able to hook into format determination to override as dictated by https://typestrong.org/ts-node/docs/module-type-overrides

Glancing at the readme, looks like it ticks at least a few of those boxes.

@cspotcode cspotcode self-requested a review October 6, 2021 19:05
Copy link
Collaborator

@cspotcode cspotcode left a comment

Choose a reason for hiding this comment

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

Looks good to go. Follow-up PRs can fill in the appropriate version numbers once the new loader API lands in node.

@cspotcode
Copy link
Collaborator

Does esbuild's sync API still have that per-call process spawning overhead? That's why I implemented swc support for ts-node, but I want to add esbuild as well once that process-spawning limitation goes away.

@jonaskello
Copy link
Contributor Author

The fix was a one-line change; tests are passing now.

Nice :-)

We want to do both CJS and ESM resolution...

The initial goal for ts-esm-resolve is for ESM resolution. Not sure how much work it would be to add CJS resolution. A lot of the logic is of course similar so it might be rather easy.

...accept a pre-parsed config or set of multiple project-referenced configs

Yes, currently it is accepting a path to a tsconfig file and resolving it to an internal Map of all referenced tsconfigs. Might be better to make that map a direct input param instead so pre-parsed config could be passed in.

...share FS cache

That should be possible since all FS calls are done through callback functions passed to the resolve function. I currently use this to mock the filesystem in the test-cases but it should also be possible to use it for cached FS calls.

@jonaskello
Copy link
Contributor Author

...hook into format determination

AFAIK the resolve process does not need to know the format, that is only needed later when executing the module?

@cspotcode
Copy link
Collaborator

Oh yeah, you're right: format only needed for load not resolve.

Might be better to make that map a direct input param instead so pre-parsed config could be passed in.

We allow ts-node-specific overrides to the compilerOptions, so we'd need a way to do that. We already apply those overrides in-memory, so as long as we can relay that info to the target library, should work. Having said that, not sure what the use case would be for overriding "paths" for ts-node.

@jonaskello
Copy link
Contributor Author

jonaskello commented Oct 7, 2021

Does esbuild's sync API still have that per-call process spawning overhead? That's why I implemented swc support for ts-node, but I want to add esbuild as well once that process-spawning limitation goes away.

I haven't dug deep into the transpilers for load. I was investigating using snowpack or vitejs and they both use esbuild. However they do not resolve typescript files fully for project references. In the end I would like to have a full-stack typescript ESM setup with both node and snowpack/vitejs supporting full typescript resolving for references/workspaces. I've been experimenting with such a setup in this repo. Currently it uses the ts-node esm loader and snowpack.

@cspotcode
Copy link
Collaborator

cspotcode commented Oct 10, 2021

Noting that I was concerned about some sourcemaps, but I can't remember why or if they're fixed or covered by tests.
#1457 (comment)

This is still an experimental API surface so I feel good merging now.

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.

Update ESM hook for consolidated node API
2 participants