-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
Import assertion support #1559
Import assertion support #1559
Conversation
For review, this diff excludes all the prettier changes: https://github.com/Pokute/ts-node/compare/d5bebe30d6e0af32c288b56e62482bd3b0787d77..import-assertion-support Are import assertions necessary to import JSON? Or is node able to import JSON without the import assertions? I'm not an expert, but I've been following along some discussion and I think the import assertions are optional; JSON can be imported without them. It's a tad confusing, but all tests are declared in |
Ah yes, this is probably due to the reasons I explained in my previous comment. Organization of our tests is a work-in-progress, so some stuff is organized, and some is not. If you hit anything that seems confusing or inconsistent, or you're not sure where to put the tests, feel free to ask here. |
At least node version 17.2.0 requires import assertions JSON importing hasn't been automatic since JSON isn't able to have any code in it and thus everyone who imports JSON should be able to rely on it being safe from remote code insertion. This is not a big issue in node environment, but for web, you should be able to import JSON directly from any URL. The web ecosystem has never relied on file extension and relies on the mimetype sent by the server (which could be malicious). This assertion makes the necessary verification on client side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to solve (#1572) the same problem before I discovered this PR so I’m offering my insights to the problem here.
src/esm.ts
Outdated
{ | ||
format, | ||
importAssertions: context.importAssertions, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this more future proof you could do the following
{ | |
format, | |
importAssertions: context.importAssertions, | |
}, | |
{ | |
...context, | |
format, | |
}, |
This code will continue to work even if more properties are added to the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aboard with that as long as I can be convinced that it's safe. I just played it over-safe initially since I didn't want to spend time considering it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I also prefer playing it safe. But I don't know what other fields might appear on the context
object. Do we know if node plans to add additional fields, and if so, what those fields are?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we limit ourselves to forwarding only known properties, but we add a unit test that probes node.js and detects when it starts including additional properties on the context object? Our test matrix runs nightly against many versions of node, including node nightly, so this will be a good way to receive advance warning when node starts including additional stuff on the context which we might want to forward?
src/esm.ts
Outdated
export type NodeImportAssertions = { | ||
type: 'json' | 'wasm'; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NodeJS documentation doesn’t specify the shape of the importAssertions
object. If it is present on the context the property type
may not be present. I’d suggest using the opaque type object
for importAssertions
. This has no effect on the behavior of the code but may prevent issues with type-safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting off topic, but while you both are available to discuss:
ts-node's resolver may need to become more complex in the future to support outDir->rootDir mappings and additional file extensions. In that spirit, do you know if import assertions ever affect the resolution of a module specifier to a filesystem path? Wondering if it would be good to have typings for the type
property or not. Otherwise I like the opaque type suggestion.
Also, I don't see type: 'wasm'
mention in node's docs anywhere. Where did you find that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if import assertions ever affect the resolution of a module specifier to a filesystem path? Wondering if it would be good to have typings for the
type
property or not. Otherwise I like the opaque type suggestion.
I don't think the assert
would be ever used for transforms. There's mentioning of import ... from 'foo' assert { ... } with { ... }
and that sounds better place to define transforms. Further, transforms might be host-specific, so specifying the in JS code itself would break code between different hosts. Such host-specific transforms would be better specified elsewhere.
Rather than granting hosts free reign to implement JSON modules independently, specifying them in TC39 guarantees that they will behave consistently across all ECMA262-compliant hosts.
I guess It's up to the host implementation to handle the type assertions, but TC39 specifically wanted to specify the type: "json"
so that the implementations would be compatible. If node or other hosts implement other types independently, the compatibility will be at greater risk.
Also, I don't see
type: 'wasm'
mention in node's docs anywhere. Where did you find that?
True! I pre-empted them when I added the --experimental-wasm-modules
flag support, but that doesn't specify the import assertion at all now that I look at it. It's about just as likely that the import assertion type would end up being webassembly
, so it might be better to drop it from the PR for now (or add both).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the fixture added to /tests
, thank you! But it still needs to be added to /src/test/*.spec.ts
. Also a couple other general questions about import assertions. Thank you in advance!
e5d7ba5
to
60c7d59
Compare
Codecov Report
|
4f64fa4
to
deaaec3
Compare
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [ts-node](https://typestrong.org/ts-node) ([source](https://github.com/TypeStrong/ts-node)) | devDependencies | minor | [`10.4.0` -> `10.5.0`](https://renovatebot.com/diffs/npm/ts-node/10.4.0/10.5.0) | --- ### Release Notes <details> <summary>TypeStrong/ts-node</summary> ### [`v10.5.0`](https://github.com/TypeStrong/ts-node/releases/v10.5.0) [Compare Source](TypeStrong/ts-node@v10.4.0...v10.5.0) <!-- I don't make a discussion thread for every release. Github has a button to make a discussion thread for a release. Then I update the discussion thread to remove the release notes and instead link to the release. --> Questions about this release? Ask in the official discussion thread: [#​1634](TypeStrong/ts-node#1634) **Added** - Eliminate "Emit Skipped" errors ([#​693](TypeStrong/ts-node#693), [#​1345](TypeStrong/ts-node#1345), [#​1629](TypeStrong/ts-node#1629)) - Avoids all "Emit Skipped" errors by performing a fallback `transpileOnly`-style transformation. - Does not affect typechecking. Type errors are still detected and thrown. - Fallback has the same limitations as `isolatedModules`. This will only affect rare cases such as using `const enums` with `preserveConstEnums` disabled. - Fixes [#​693](TypeStrong/ts-node#693) - Graduate swc transpiler out of experimental; add `swc: true` convenience option ([docs](https://typestrong.org/ts-node/docs/transpilers)) ([#​1487](TypeStrong/ts-node#1487), [#​1536](TypeStrong/ts-node#1536), [#​1613](TypeStrong/ts-node#1613), [#​1627](TypeStrong/ts-node#1627)) - `"swc": true` or `--swc` will use swc for faster execution - This feature is no longer marked "experimental." Thank you to everyone who filed bugs! - swc transpiler attempts to load `@swc/core` or `@swc/wasm` dependencies from your project before falling-back to global installations ([#​1613](TypeStrong/ts-node#1613), [#​1627](TypeStrong/ts-node#1627)) - global fallback only occurs when using a global installation of ts-node - Add support for TypeScript's `traceResolution` output ([docs](https://www.typescriptlang.org/tsconfig/#traceResolution)) ([#​1128](TypeStrong/ts-node#1128), [#​1491](TypeStrong/ts-node#1491)) [@​TheUnlocked](https://github.com/TheUnlocked) - Support import assertions in ESM loader ([docs](https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#import-assertions)) ([#​1557](TypeStrong/ts-node#1557), [#​1558](TypeStrong/ts-node#1558), [#​1559](TypeStrong/ts-node#1559), [#​1573](TypeStrong/ts-node#1573)) [@​Pokute](https://github.com/Pokute), [@​geigerzaehler](https://github.com/geigerzaehler) - Allows importing JSON files from ESM with the requisite flag ([docs](https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#json-modules)) - `ts-node -vvv` also logs absolute paths to `ts-node` and `typescript`, to make it more obvious when you're accidentally using globally-installed versions ([#​1323](TypeStrong/ts-node#1323), [#​1620](TypeStrong/ts-node#1620)) - Add swc target "es2022" ([#​1535](TypeStrong/ts-node#1535), [#​1540](TypeStrong/ts-node#1540)) - When you have target es2022 in tsconfig, will use swc's es2022 target **Changed** - Initialize TypeScript compiler before starting REPL prompt ([#​1498](TypeStrong/ts-node#1498)) [@​TheUnlocked](https://github.com/TheUnlocked) - Improves responsiveness for first line of REPL input - Use `v8-compile-cache-lib` to load typescript - improves startup time ([#​1339](TypeStrong/ts-node#1339), [#​1603](TypeStrong/ts-node#1603)) - Support both `--camelCase` and `--hyphen-case` for all CLI flags; update documentation to use `--camelCase` ([#​1598](TypeStrong/ts-node#1598), [#​1599](TypeStrong/ts-node#1599)) - Not a breaking change; CLI continues to accept both forms - Make `TSError` `diagnosticText` property non-enumerable to prevent it from being logged below the stack ([#​1632](TypeStrong/ts-node#1632)) **Fixed** - Fix [#​1538](TypeStrong/ts-node#1538): REPL inputs fail to transpile via swc ([#​1538](TypeStrong/ts-node#1538), [#​1541](TypeStrong/ts-node#1541), [#​1602](TypeStrong/ts-node#1602)) - Fix [#​1478](TypeStrong/ts-node#1478): REPL erroneously logged `undefined` for all inputs after the first when using swc transpiler ([#​1478](TypeStrong/ts-node#1478), [#​1580](TypeStrong/ts-node#1580), [#​1602](TypeStrong/ts-node#1602)) - Fix [#​1389](TypeStrong/ts-node#1389): In `--showConfig` output, emit accurate `moduleTypes` paths resolved relative to the `tsconfig.json` which declared them ([#​1389](TypeStrong/ts-node#1389), [#​1619](TypeStrong/ts-node#1619)) - Fix: Remove indentation from `ts-node --help` output ([#​1597](TypeStrong/ts-node#1597), [#​1600](TypeStrong/ts-node#1600)) - Fix [#​1425](TypeStrong/ts-node#1425): Merged definitions correctly into `tsconfig.schemastore-schema.json` ([#​1425](TypeStrong/ts-node#1425), [#​1618](TypeStrong/ts-node#1618)) - Fix: Allow disabling `"use strict"` emit in SWC transpiler ([#​1531](TypeStrong/ts-node#1531), [#​1537](TypeStrong/ts-node#1537)) - Fix: Add missing `ERR_UNKNOWN_FILE_EXTENSION` constructor; was throwing `ERR_UNKNOWN_FILE_EXTENSION is not a constructor` ([#​1562](TypeStrong/ts-node#1562)) [@​bluelovers](https://github.com/bluelovers) - Fix [#​1565](TypeStrong/ts-node#1565): entrypoint resolution failed on node v12.0.x and v12.1.x ([#​1565](TypeStrong/ts-node#1565), [#​1566](TypeStrong/ts-node#1566)) [@​davidmurdoch](https://github.com/davidmurdoch) #### Docs - Explain `env -S` flag for shebangs ([docs](https://typestrong.org/ts-node/docs/usage#shebang)) ([#​1448](TypeStrong/ts-node#1448), [#​1545](TypeStrong/ts-node#1545)) [@​sheeit](https://github.com/sheeit), [@​chee](https://github.com/chee) - Suggest `skipIgnore` when you want to compile files in node_modules ([docs](https://typestrong.org/ts-node/docs/how-it-works)) ([#​1553](TypeStrong/ts-node#1553)) [@​webstrand](https://github.com/webstrand) - Fix typo in `moduleTypes` on options page ([docs](https://typestrong.org/ts-node/docs/options)) ([#​1630](TypeStrong/ts-node#1630), [#​1633](TypeStrong/ts-node#1633)) #### Misc - Adds experimental `experimentalResolverFeatures` option, but it does not do anything yet ([#​1514](TypeStrong/ts-node#1514), [#​1614](TypeStrong/ts-node#1614)) https://github.com/TypeStrong/ts-node/milestone/4 </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1156 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
I am having difficulty using import assertions with ts-node when swc is enabled. Import assertions work just fine when swc is disabled, but not when swc is enabled. Import assertions seem to be behind a config flag for swc per swc-project/swc#2802.
but I can't find any way to set importAssertions to true. I can see that ts-node creates its own swcconfig from the tsconfig here and I don't really see anyway to override or add to these options. Am I missing something? Should this be enabled by default? I would be happy to move this to a new issue or create a pr if you fell that would be more appropriate. |
I have the exact the same problem, the .swcrc file is ignored so setting |
EDIT from @cspotcode: maintainer notes: credit @geigerzaehler in release notes, for help reviewing (see also #1572)
This PR implements import assertion support for json and wasm.
This builds on another PRs ( #1557 (updating typescript to 4.5.2) and #1558 (updating prettier and the prettified code) that updates required packages. These prior PRs are required to support import assertion syntax.
You can use
to import json data directly in your code!
Import assertions for JSON are for making sure that what you're importing is not code (and thus immediately executed upon import). This is most important for web where you might not trust the server you're importing from.
There's no tests for wasm importing.
Honestly I'm surprised that the tests seem to pass since I would think that
--experimental-json-modules
flag is needed for node. But nothing seems to fail so I'm not sure about the tests running correctly.