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

Add import assertions to type only imports and import types to force the resolution mode of the specifier #47807

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Feb 8, 2022

This PR adds support for a TS-specific assertion on type-only imports and on import type nodes which forces the resolver into either require or import resolution mode, allowing access to types which the containing file's default mode would normally make difficult or impossible to access. They look like this:

import type { RequireInterface } from "pkg" assert { "resolution-mode": "require" };
import type { ImportInterface } from "pkg" assert { "resolution-mode": "import" };

export interface LocalInterface extends RequireInterface, ImportInterface {}

export type LocalType =
    & import("pkg", { assert: {"resolution-mode": "require"} }).RequireInterface
    & import("pkg", { assert: {"resolution-mode": "import"} }).ImportInterface;

This is essentially a continuation of #47732 that allows resolver configurability for more kinds of type-based imports. Since it's included in this PR, it's probably best to go and review & merge that first.

Fixes #47338 by allowing the types of an esm package to be fetched in a cjs file via these modal imports.
Fixes #47248

@typescript-bot typescript-bot assigned weswigham and unassigned weswigham Feb 8, 2022
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 8, 2022
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Feb 10, 2022
@TimvdLippe
Copy link
Contributor

FYI the PR description claims "Fixes #47807", but this PR has number 47807

@weswigham weswigham force-pushed the import-modal-assertions branch from 45e3c3d to 355ae11 Compare February 15, 2022 21:55
@weswigham weswigham force-pushed the import-modal-assertions branch from 355ae11 to 476dc6d Compare February 15, 2022 22:03
@weswigham weswigham removed the Fix Available A PR has been opened for this issue label Feb 15, 2022
src/compiler/program.ts Outdated Show resolved Hide resolved
src/compiler/types.ts Outdated Show resolved Hide resolved
@weswigham weswigham merged commit ea0db9e into microsoft:main Mar 2, 2022
@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Mar 30, 2022

This feels really wrong 😕

The proposal explicitly says that import assertions must not affect how a module is evaluated or resolved:

[...]
The implementation of HostResolveImportedModule must conform to the following requirements:

  • [...]
  • moduleRequest.[[Assertions]] must not influence the interpretation of the module or the module specifier; instead, it may be used to determine whether the algorithm completes normally or with an abrupt completion.

In other words, if both these two imports succeed they must resolve/evaluate the same file:

import { A } from "pkg" assert { "resolution-mode": "require" };
import { A } from "pkg" assert { "resolution-mode": "import" };

The imports assertions proposal even mentions a possible follow-up proposal that would allow what you need, but it would be a separate proposal (with likely different syntax).

I know that this PR is only about import type so it's technically "just TS" and not JS, but actively going against the JS semantics should be discouraged.

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Mar 30, 2022

https://github.com/tc39/proposal-import-reflection is probably what you need, even if it's not clear yet if it's also meant to affect resolution.

@weswigham
Copy link
Member Author

weswigham commented Mar 30, 2022

These don't affect runtime and are type-only (your example above doesn't work - you have to say import type); it's fine - it's much better than introducing full on new syntax.

@nayeemrmn
Copy link

it's fine - it's much better than introducing full on new syntax.

@weswigham This is wrong, we're using the assert keyword for something that isn't an assertion. Why? You're correct that this is a type-only statement, all the more reason to do literally anything else and not misuse a JS syntax.

There are plenty of other options that make sense without completely new and ts-only syntax e.g.

// comment directive, nothing new

// @ts-resolution=require
import type { RequireInterface } from "pkg";
// https://github.com/tc39/proposal-import-reflection

import type { RequireInterface } from "pkg" as "resolution=require";
// dangles off the already ts-only part

import type(resolution=require) { RequireInterface } from "pkg";

Please consider reverting this change.

@nayeemrmn
Copy link

I've already seen a lot of people confused by import assertions and what they could be used for in runtimes, specifically conflating them with what's proposed in https://github.com/tc39/proposal-import-reflection. It would be a shame to see TS fostering that misconception, or perhaps falling victim to it.

@glen-84
Copy link

glen-84 commented Apr 9, 2022

Why is it:

assert {"resolution-mode": "import"};

(requiring quotes), and not:

assert {resolutionMode: "import"};

?

@weswigham
Copy link
Member Author

Consistency with the triple-slash reference form and a bit of extra syntax encumbrance to make you pause and think if you really need to use it. :)

@frehner
Copy link

frehner commented Oct 8, 2022

Would this also hypothetically help solve the issue found at #46213 ? If so, how soon do you think this feature will graduate from nightly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

error importing type from esm in commonjs Allow synchronous type-only import of ESM package from CJS
9 participants