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

Import assertion support #1559

Merged
merged 7 commits into from Jan 21, 2022
Merged

Conversation

Pokute
Copy link
Contributor

@Pokute Pokute commented Dec 1, 2021

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

import carData from './car.json' assert { type: 'json' };
const { default: dynamicCarData } = await import('./car.json', { assert: { type: 'json' }});

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.

@cspotcode
Copy link
Collaborator

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 /src/tests. The bulk of the tests invoke ts-node in some way, often telling it to run the test projects declared in /tests. So this PR will need to have the test added to /src/tests. It will probably need to do a version number check so that it only runs on newer TS versions. We also run the tests against older versions of TS to ensure compatibility.

@cspotcode
Copy link
Collaborator

cspotcode commented Dec 1, 2021

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.

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.

@Pokute
Copy link
Contributor Author

Pokute commented Dec 2, 2021

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.

At least node version 17.2.0 requires import assertions TypeError [ERR_IMPORT_ASSERTION_TYPE_MISSING]: Module "file:///workspaces/Advent%20of%20Code%202021/inputs/1.json" needs an import assertion of type "json". Without the --experimental-json-modules the error is TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".json" for ...

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.

Copy link

@geigerzaehler geigerzaehler left a 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
Comment on lines 189 to 197
{
format,
importAssertions: context.importAssertions,
},

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

Suggested change
{
format,
importAssertions: context.importAssertions,
},
{
...context,
format,
},

This code will continue to work even if more properties are added to the context.

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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
Comment on lines 89 to 91
export type NodeImportAssertions = {
type: 'json' | 'wasm';
};

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.

Copy link
Collaborator

@cspotcode cspotcode Dec 23, 2021

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?

Copy link
Contributor Author

@Pokute Pokute Dec 23, 2021

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.

From JSON modules proposal

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

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.

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!

tests/esm-import-assertions/importJson.ts Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 26, 2021

Codecov Report

Merging #1559 (84efc82) into main (a817289) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted Files Coverage Δ
dist-raw/node-options.js 78.37% <ø> (ø)
src/esm.ts 84.21% <ø> (ø)
dist-raw/node-esm-default-get-format.js 74.28% <0.00%> (+2.85%) ⬆️

@cspotcode cspotcode merged commit 89f88eb into TypeStrong:main Jan 21, 2022
@cspotcode cspotcode added this to the next milestone Jan 21, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 16, 2022
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: [#&#8203;1634](TypeStrong/ts-node#1634)

**Added**

-   Eliminate "Emit Skipped" errors ([#&#8203;693](TypeStrong/ts-node#693), [#&#8203;1345](TypeStrong/ts-node#1345), [#&#8203;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 [#&#8203;693](TypeStrong/ts-node#693)
-   Graduate swc transpiler out of experimental; add `swc: true` convenience option ([docs](https://typestrong.org/ts-node/docs/transpilers)) ([#&#8203;1487](TypeStrong/ts-node#1487), [#&#8203;1536](TypeStrong/ts-node#1536), [#&#8203;1613](TypeStrong/ts-node#1613), [#&#8203;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 ([#&#8203;1613](TypeStrong/ts-node#1613), [#&#8203;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)) ([#&#8203;1128](TypeStrong/ts-node#1128), [#&#8203;1491](TypeStrong/ts-node#1491)) [@&#8203;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)) ([#&#8203;1557](TypeStrong/ts-node#1557), [#&#8203;1558](TypeStrong/ts-node#1558), [#&#8203;1559](TypeStrong/ts-node#1559), [#&#8203;1573](TypeStrong/ts-node#1573)) [@&#8203;Pokute](https://github.com/Pokute), [@&#8203;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 ([#&#8203;1323](TypeStrong/ts-node#1323), [#&#8203;1620](TypeStrong/ts-node#1620))
-   Add swc target "es2022" ([#&#8203;1535](TypeStrong/ts-node#1535), [#&#8203;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 ([#&#8203;1498](TypeStrong/ts-node#1498)) [@&#8203;TheUnlocked](https://github.com/TheUnlocked)
    -   Improves responsiveness for first line of REPL input
-   Use `v8-compile-cache-lib` to load typescript
    -   improves startup time ([#&#8203;1339](TypeStrong/ts-node#1339), [#&#8203;1603](TypeStrong/ts-node#1603))
-   Support both `--camelCase` and `--hyphen-case` for all CLI flags; update documentation to use `--camelCase` ([#&#8203;1598](TypeStrong/ts-node#1598), [#&#8203;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 ([#&#8203;1632](TypeStrong/ts-node#1632))

**Fixed**

-   Fix [#&#8203;1538](TypeStrong/ts-node#1538): REPL inputs fail to transpile via swc ([#&#8203;1538](TypeStrong/ts-node#1538), [#&#8203;1541](TypeStrong/ts-node#1541), [#&#8203;1602](TypeStrong/ts-node#1602))
-   Fix [#&#8203;1478](TypeStrong/ts-node#1478): REPL erroneously logged `undefined` for all inputs after the first when using swc transpiler ([#&#8203;1478](TypeStrong/ts-node#1478), [#&#8203;1580](TypeStrong/ts-node#1580), [#&#8203;1602](TypeStrong/ts-node#1602))
-   Fix [#&#8203;1389](TypeStrong/ts-node#1389): In `--showConfig` output, emit accurate `moduleTypes` paths resolved relative to the `tsconfig.json` which declared them ([#&#8203;1389](TypeStrong/ts-node#1389), [#&#8203;1619](TypeStrong/ts-node#1619))
-   Fix: Remove indentation from `ts-node --help` output ([#&#8203;1597](TypeStrong/ts-node#1597), [#&#8203;1600](TypeStrong/ts-node#1600))
-   Fix [#&#8203;1425](TypeStrong/ts-node#1425): Merged definitions correctly into `tsconfig.schemastore-schema.json` ([#&#8203;1425](TypeStrong/ts-node#1425), [#&#8203;1618](TypeStrong/ts-node#1618))
-   Fix: Allow disabling `"use strict"` emit in SWC transpiler ([#&#8203;1531](TypeStrong/ts-node#1531), [#&#8203;1537](TypeStrong/ts-node#1537))
-   Fix: Add missing `ERR_UNKNOWN_FILE_EXTENSION` constructor; was throwing `ERR_UNKNOWN_FILE_EXTENSION is not a constructor` ([#&#8203;1562](TypeStrong/ts-node#1562)) [@&#8203;bluelovers](https://github.com/bluelovers)
-   Fix [#&#8203;1565](TypeStrong/ts-node#1565): entrypoint resolution failed on node v12.0.x and v12.1.x ([#&#8203;1565](TypeStrong/ts-node#1565), [#&#8203;1566](TypeStrong/ts-node#1566)) [@&#8203;davidmurdoch](https://github.com/davidmurdoch)

#### Docs

-   Explain `env -S` flag for shebangs ([docs](https://typestrong.org/ts-node/docs/usage#shebang)) ([#&#8203;1448](TypeStrong/ts-node#1448), [#&#8203;1545](TypeStrong/ts-node#1545)) [@&#8203;sheeit](https://github.com/sheeit), [@&#8203;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)) ([#&#8203;1553](TypeStrong/ts-node#1553)) [@&#8203;webstrand](https://github.com/webstrand)
-   Fix typo in `moduleTypes` on options page ([docs](https://typestrong.org/ts-node/docs/options)) ([#&#8203;1630](TypeStrong/ts-node#1630), [#&#8203;1633](TypeStrong/ts-node#1633))

#### Misc

-   Adds experimental `experimentalResolverFeatures` option, but it does not do anything yet ([#&#8203;1514](TypeStrong/ts-node#1514), [#&#8203;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>
@leonitousconforti
Copy link

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.

The import assertions should stay intact if the target environment is set to esnext, module system is esm and importAssertions in the config is set to true.

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.

@vinczebalazs
Copy link

I have the exact the same problem, the .swcrc file is ignored so setting keepImportAssetions does not seem to have any effect.

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.

None yet

5 participants