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

New loader hooks api #13

Closed
TrySound opened this issue Oct 20, 2021 · 10 comments · Fixed by #16
Closed

New loader hooks api #13

TrySound opened this issue Oct 20, 2021 · 10 comments · Fixed by #16

Comments

@TrySound
Copy link

Node 16 just landed new loader hooks api. Is tsm compatible with it?
https://github.com/nodejs/node/releases/tag/v16.12.0

@lukeed
Copy link
Owner

lukeed commented Oct 20, 2021

I assume not since it was released 1 hour ago and has been raised in the "Notable Changes" section 😄

@TrySound
Copy link
Author

Node 17 got it first yesterday 😄

@lukeed
Copy link
Owner

lukeed commented Oct 20, 2021

I'll take a look later today or tomorrow. Specifically I'm interested in if this is backwards compatible at all (notes mention deprecation warning for old hooks, but does that mean feature loss?) and if this is likely the final design. I know it's experimental, but reworking tsm every release will make tsm versioning difficult

@karlhorky
Copy link

karlhorky commented Oct 22, 2021

Yeah, it breaks (saw this first on CI, because GitHub Actions with node-version: '16' upgraded to 16.12.0 silently):

$ tsm index.ts
(node:3063) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /home/runner/work/project/index.ts
    at new NodeError (node:internal/errors:371:5)
    at Object.file: (node:internal/modules/esm/get_format:72:15)
    at defaultGetFormat (node:internal/modules/esm/get_format:85:38)
    at defaultLoad (node:internal/modules/esm/load:13:42)
    at ESMLoader.load (node:internal/modules/esm/loader:303:26)
    at ESMLoader.moduleProvider (node:internal/modules/esm/loader:230:58)
    at new ModuleJob (node:internal/modules/esm/module_job:63:26)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:244:11)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

@karlhorky
Copy link

karlhorky commented Oct 26, 2021

Apparently this is the PR with the new hooks implementation in ts-node: TypeStrong/ts-node#1457

@lukeed
Copy link
Owner

lukeed commented Oct 26, 2021

Thanks but the implementation isn't the obstacle. It was supporting both loader APIs without inducing a breaking change. It dawned on me how to do it last night so will implement shortly.

This way, the majority of users with older 16 versions can continue using it, and CI/bleeding edge installs can use the new one.

This feels like the best (and necessary) approach since there's no 16 LTS yet for me to point to as a firm requirement.

@PabloSzx
Copy link

PabloSzx commented Oct 26, 2021

Thanks but the implementation isn't the obstacle. It was supporting both loader APIs without inducing a breaking change. It dawned on me how to do it last night so will implement shortly.

This way, the majority of users with older 16 versions can continue using it, and CI/bleeding edge installs can use the new one.

This feels like the best (and necessary) approach since there's no 16 LTS yet for me to point to as a firm requirement.

I adapted it using semver to detect the Node.js version and doing:

HAS_UPDATED_HOOKS
  ? undefined
  : async function (...

Check https://github.com/PabloSzx/bob-esbuild/blob/main/packages/bob-tsm/src/loader.ts for that and how the load hook works, bob-tsm works perfectly in both <16.12.0 and >=16.12.0

PS: btw, I created bob-tsm using tsm as inspiration but adding opinionated changes, like watch mode, esbuild as peer dependency, and extra cli options

@lukeed
Copy link
Owner

lukeed commented Oct 26, 2021

Yes that's what it will be.

Thanks but PS I should be mentioned in the license for that package.

lukeed added a commit that referenced this issue Oct 27, 2021
lukeed added a commit that referenced this issue Oct 27, 2021
* chore(loader): rename `load` ~> `toConfig` helper

* chore(loader): add deprecated note to self

* fix(loader): add new `load` hook for new API design;

- Closes #13
@karlhorky
Copy link

karlhorky commented Oct 28, 2021

Thanks @lukeed ! 🙌

@TrySound
Copy link
Author

Thanks, very cool!

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 a pull request may close this issue.

4 participants