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

Consider redesigning "resolution-mode" to not be an assertion since it's not an assertion #48644

Closed
dsherret opened this issue Apr 11, 2022 · 25 comments
Assignees
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@dsherret
Copy link
Contributor

Suggestion

The latest TS 4.7 has a new "resolution-mode" feature (https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-beta/#resolution-mode) that can be used to select which module resolution to use:

import type { TypeFromRequire } from "pkg" assert {
    "resolution-mode": "require" // or "import"
};

I understand this is "ok" by the spec police because it appears in an import type, but this is not an assertion so it appearing in an assert clause doesn't seem ideal.

Note: I say this is not an assertion because import assertions assert something about the module. This instead seems to be a module resolution directive... maybe I'm mistaken though.

🔍 Search Terms

resolution-mode import assertions

⭐ Suggestion

I think @nayeemrmn's suggestion for using a comment directive seems more ideal #47807 (comment):

// @ts-resolution="require"
import type { RequireInterface } from "pkg";

...but that's not as conveinent when used in a type like shown in the example:

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

Perhaps this should be?

export type LocalType =
    & require("pkg").RequireInterface
    & import("pkg").ImportInterface;

Or, perhaps still using a comment directive:

export type LocalType =
    & /* @ts-resolution="require" */ import("pkg").RequireInterface
    & import("pkg").ImportInterface;
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Apr 11, 2022
@weswigham
Copy link
Member

We repurpose object literal syntax in type space to represent object literal types, I don't see why repurposing assertion syntax in type space for representing options for resolution is meaningfully different. No effect on the runtime, completely separated from any runtime guarantees, and on the surface level, looks and behaves exactly like assertions do (as a kind of option bag for the import).

@nayeemrmn
Copy link

We repurpose object literal syntax in type space to represent object literal types, I don't see why repurposing assertion syntax in type space for representing options for resolution is meaningfully different.

In the first case you are repurposing curly braces and other punctuation, in the second you are abusing the assert keyword for what is actually a selection. It's like making a function:

function assertResolutionRequire(importDecl) {
  importDecl.resolution = "require";
}

and on the surface level, looks and behaves exactly like assertions do

No, if you were to rewrite some assertion you expect that the only change in outcome should be the pass or failure of that assertion. It is confusing and dangerous for other semantics to change while the word assert is explicitly used.

And again it's not like there was ever some radical advantage of this syntax.

like assertions do (as a kind of option bag for the import)

This is a misconception to shift away from. Import assertions are not meant to be an options bag.

@weswigham
Copy link
Member

abusing the assert keyword for what is actually a selection

Yes, asserting that the specifier resolution operation was carried out under some rule set, which is always true in this case specifically because said assertion was used. 😊 The distinction is moot to us in the type system, since we don't deal with the same cache-key nonsense.

And again it's not like there was ever some radical advantage of this syntax.

There is! The AST nodes are the same as the non-type versions of the call, so tools like eslint don't need to explicitly handle any new syntax because they already handle it for the non-type syntactic forms.

This is a misconception to shift away from. Import assertions are not meant to be an options bag.

I think that's an argument that may hold water with engine authors willing to argue implementation minutiae, but is a tough sell in general to the average user. Heck, the syntactic differences between import alias {} and object destructuring {} is already something that confuses beginners, and if I had to explain to every user why there were two seemingly identical yet mutually exclusive options bags on imports (with mutually exclusive members)... without getting hyperbolic, let's just say I'd have a bad time. Even if the type forms were exactly the same but said notassert instead of assert (much to the chagrin of linters and people who like AST stability) I'd still be fielding questions like "Isn't this just an assertion?" from everyone except engine authors forever, and every user familiar with assertions would mistakenly put it in assertion objects by accident from muscle memory.

@lucacasonato
Copy link

fyi: i started writing this comment an hour ago, so i have not incorporated comments posted in the last hour ago :-)

looks and behaves exactly like assertions do (as a kind of option bag for the import)

I think this is where the misunderstanding is coming from. Import assertions are NOT options bags for imports. Let me elaborate:

In ES, building of the module graph (often referred to as module loading) can be thought of as three distinct stages: resolution, asset loading, and asset evaluation.

  • Resolution is the first step of resolution. It takes the string entered into the import statement, and the module specifier of the importing module (the referrer), and "resolves" them into a new specifier string.
  • Asset loading takes the module specifier produced by the resolver, and loads the corresponding asset. This could be a file on disk, a network resource, an internal binding or anything else.
  • Evaluation takes the asset and turns it into a module record, which has a linked module namespace, which are all of the items exported by this module.
Specification background / sources

The ECMA262 combines all of these three steps into a single HostResolveImportedModule host AO. It takes the specified specifier string from the import, and the optional referring module record and returns a new module record.

The split of steps as described above is enforced in the spec anyway though the following normative requirement on implementations of HostResolveImportedModule:

Each time this operation is called with a specific referencingScriptOrModule, specifier pair as arguments it must return the same Module Record instance if it completes normally.

Import assertions slot into this by adding three more steps:

  • Resolution is the same as above. Just the import specifier an referrer pair are used to determine the resolved specifier.
  • Resolution assertion is a new step that takes the resolved specifier and the assertions and can either succeed or fail. It can not modify the specifier.
  • Asset loading works the same way as above. It takes a specifier and returns an asset.
  • Asset assertion is a new step that takes the asset and the assertions and can either succeed or fail. It can not modify the asset.
  • Evaluation works exactly like above. It takes the asset and returns a module record.
Specification background / sources

As mentioned earlier, ECMA262 combines all of these steps into a single host AO HostResolveImportedModule. Import assertions changes the signature of this AO to include the assertions as an argument to this AO argument.

The import assertions proposal also adds a new requirement to the host AO though:

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.

This new requirement enforces the separation between step 1 and step 2, and between step 3 and step 4.

The confusion stems around it looking like that the assertion tells an engine how a given module is to be evaluated. This is NOT the case though. For web browsers / Deno (which use the module loading algorithms from the HTML spec) for example, when a user performs a JSON import (import data from "./data.json" assert { type: "json" };) the browser does not look at the assertion to determine how to interpret the given file at all. It looks at the content-type header (or file extension for local files) to determine what "type" an asset is. After the asset is already fully loaded, the assertion is used to check that the "type" that was resolved from the content-type (or file extension) matches the "type" specified in the assertion. The assertion asserts that the type matches. It does not tell the browser what type of file an asset is.

So now with the spec details out of the way, back to the problem at hand:

TypeScript uses the assertion to modify how resolution is performed. This is a violation of the ECMA262 spec. If that in itself is "bad" is dependant on if you think TypeScript should adhere to the spec closely or not, but that is not my concern right now. What I do think is really concerning is that TypeScript is setting the precedent that an assertion on a statement that for all intents an purposes looks to the average Joe like an ES import statement may be used to affect resolution, module loading, or evaluation. This is a bad precent in my opinion:

  • Users see that assertions can affect resolution, loading, or evaluation in tooling, and think that assertions on import statements also affect these steps in runtimes (which is not the case).
  • There are other proposals in the works at TC39 to allow for specifying other properties on import statements to let users add additional options to import statements that can then be used to give extra options to certain steps in the module graph building steps. For example the import reflection proposal adds syntax space for options that can modify evaluation (I am championing this proposal).
  • There is currently no sign from TC39 that syntax space is being considered that would affect resolution or loading. As such these two should never be based on anything other than the specifier.

Thank you for your consideration.

@lucacasonato
Copy link

To elaborate some more on my above comment:

Users see that assertions can affect resolution, loading, or evaluation in tooling, and think that assertions on import statements also affect these steps in runtimes (which is not the case).

I think this is really critical. Users need to understand that import data from "./data_that_is_json.txt" assert { type: "json" } will never work because the content type (or extension for local files) is not application/json for .txt files in most cases. TypeScript is blurring this line (not in a good way).

@DanielRosenwasser
Copy link
Member

This is a violation of the ECMA262 spec.

Can we avoid making statements like this? import type is not an ECMA-262 construct. As someone working on the type annotations proposal which hopes to include import type, the intent is for import type to not include any runtime resolution either.

So I would rather have us focus on

  1. Whether this is really a point of confusion for users for regular imports (I don't think so - you should get an error if you try to use these on regular imports)
  2. Whether this is an issue for existing tools (is it?)
  3. If there's a better syntax so that we can provide constructive suggestions if this is really distasteful.

@dsherret
Copy link
Contributor Author

dsherret commented Apr 13, 2022

We repurpose object literal syntax in type space to represent object literal types, I don't see why repurposing assertion syntax in type space for representing options for resolution is meaningfully different.

I don't think this is a similar comparison. Object literal types exists in the type domain in a non-analogous to JS position whereas this new "resolution-mode" non-assertion appears in an assertion clause within an import declaration that looks almost syntactically identical to a regular import declaration other than a type keyword. It's true that putting it in an import type means this new "resolution-mode" is also in the type domain, but its in the type domain in a position that doesn't align with the meaning or purpose of an import declaration's assertion clause in JS.

If there's a better syntax so that we can provide constructive suggestions if this is really distasteful.

I'm wondering about the issue with a comment directive or only allowing something like require instead of import to force require (so not allowed from import type)?

export type LocalType = require("pkg").RequireInterface

Can we avoid making statements like this? import type is not an ECMA-262 construct.

I think Luca was just making the point that import assertions aren't an options bag.

@lucacasonato
Copy link

lucacasonato commented Apr 13, 2022

Can we avoid making statements like this?

Yeah, sorry, I tried to elaborate on this in the sentence following that one. It kind of depends on if you consider import type to be a different declaration to import, or a modifier on the ES ImportDeclaration. import { type bla } ... sorta blurred this line for me. I don't think that invalidates the other points I gave later my comment though.

Whether this is an issue for existing tools.

I can comment on this. This will to some extent be an issue for Deno, yes. Our general purpose module resolution system that we use for both the Deno runtime, bundling, and static analysis tools like deno_doc (deno_graph) does not pass import assertions to the resolution callback. With this change we'll need to either modify our resolution hook to pass in module assertions (which I find rather distasteful considering they must not be used during runtime resolution anyway), or we need to provide a separate new resolution hook for types that pass in assertions and can do different resolution in import type. This also seems pretty weird.

It also requires more mental overhead for users when parsing import-like statements. They can now not expect resolution to be purely based on specifier and referrer anymore. They must now also consider assertions when figuring out what a file resolves to.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 13, 2022

Don't you need to pass something extra in anyway if there's a new syntactic construct or a leading comment?

@lucacasonato
Copy link

I might be misunderstanding, but I don't see why there would need to be a new syntactic construct or leading comment. Doesn't @dsherret's suggestion have the same effect as the new feature, but in existing syntax?

@weswigham
Copy link
Member

weswigham commented Apr 13, 2022

Semantic comments are pretty much a nonstarter for meaningful type syntax - comments that aren't /// references are supposed to be erasable (even in declaration files). import types already mean "the mode of the file", so the import vs require type is a nonstarter since it changes how import types are interpreted in a breaking way. Additionally, an options bag is useful because we may need to specify the custom conditions used the resolve the import in the future, too, and so would need another key in the options bag. The only alternative we'd consider is pretty much replacing assert with some other keyword but otherwise behaving exactly the same as far as we're concerned, but that's a needless distinction for our users and API consumers...

@weswigham
Copy link
Member

Our general purpose module resolution system that we use for both the Deno runtime, bundling, and static analysis tools like deno_doc (deno_graph) does not pass import assertions to the resolution callback. With this change we'll need to either modify our resolution hook to pass in module assertions (which I find rather distasteful considering they must not be used during runtime resolution anyway), or we need to provide a separate new resolution hook for types that pass in assertions and can do different resolution in import type. This also seems pretty weird.

How do you handle detecting the mode of a node-style import in your node compat codepath? (Eg, if it's a require or an import) - however you do that is how you should flow in this flag, since it exactly swaps that; that's what we do. Node's "two specifiers resolve form the same file to different things depending on how they're called" is diametrically opposed to ecma262 if you try to use a unified cache for cjs and esm (spoiler: node duplicates items across both caches if visible in both and periodically syncs them on demand rather than having a unified cache), so I'd wish you luck if you haven't implemented support for it yet.

@weswigham
Copy link
Member

They can now not expect resolution to be purely based on specifier and referrer anymore.

In node, they're already not. They depend on the nearest context package.json. It's very opaque.

@dsherret
Copy link
Contributor Author

@weswigham thanks for the clarification on the // comment directives not being viable and that makes sense.

import types already mean "the mode of the file", so the import vs require type is a nonstarter since it changes how import types are interpreted in a breaking way.

I see, because import(...) in an existing file that's cjs resolution currently resolves using cjs resolution so there would be no way to override that to use esm.

The only alternative we'd consider is pretty much replacing assert with some other keyword but otherwise behaving exactly the same as far as we're concerned, but that's a needless distinction for our users and API consumers...

I don't agree this is a needless distinction since "resolution-mode" is not an assertion.

For a possible alternative keyword, I kind of like with:

import type { TypeFromRequire } from "pkg" with {
    "resolution-mode": "require"
};

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

@weswigham
Copy link
Member

with already is already a reserved word because of non-strict-mode with statements and is waaaaaay more likely to conflict with future import proposals we might need to retain in type-space.

@RyanCavanaugh
Copy link
Member

I don't agree this is a needless distinction since "resolution-mode" is not an assertion.

Sure, but nothing in an import type declaration is anything at all in the first place.

It's already the case that you can write things in TypeScript-only positions that use the same syntax as JavaScript keywords but have different semantics:

  • let x: typeof y = typeof y uses two different meanings of typeof
  • let x: { [id]: 0 } = { [id]: 0 } uses the computed property name syntax in two different ways which roughly align, but not precisely
  • Same for the utterance { x: string }, which means one thing in a TypeScript-specific position and another thing in a general expression position
  • interface A extends B { has a similar-yet-different meaning than class C extends D

So for the TypeScript-specific construct import type, what's the right way to understand that reusing the assert keyword is confusing/invalid in a way that our reuse of the typeof keyword in let x: typeof y wasn't?

@lucacasonato
Copy link

lucacasonato commented Apr 13, 2022

So for the TypeScript-specific construct import type, what's the right way to understand that reusing the assert keyword is confusing/invalid in a way that our reuse of the typeof keyword in let x: typeof y wasn't?

I think they key is that with typeof you get a type of something (a binding) in both positions. With assert you assert in the regular import position, but modify in the type import position. These are conceptually different operations that you are performing.

@weswigham
Copy link
Member

From a user perspective blocking the import entirely and modifying it are both just different flavors of modification - the distinction is artificial.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 13, 2022

Let's say that we added this to the documentation

import type will search for the specified module under all possible resolution algorithms. If the algorithm that successfully found the module is not one specified by the "resolution-mode" assert, then an error is raised

As a TS-only declaration, we're allowed to specify the semantics of what import type ... "specifier" ... means, and adding an assert about which algorithm succeeded seems like a valid use of it. What conceptual violation has occurred?

@lucacasonato
Copy link

Well a violation would occur if a user has imports with different resolution modes specified for the same resolved specifier. So if that is explicitly forbidden, it seems like a possible resolution to the issue.

I'm telling you... import assertions were a mistake :(

@nayeemrmn
Copy link

Some key points I want to touch on from #48644 (comment):

The distinction is moot to us in the type system, since we don't deal with the same cache-key nonsense.

There is! The AST nodes are the same as the non-type versions of the call, so tools like eslint don't need to explicitly handle any new syntax because they already handle it for the non-type syntactic forms.

I think that's an argument that may hold water with engine authors willing to argue implementation minutiae, but is a tough sell in general to the average user ... I'd still be fielding questions like "Isn't this just an assertion?" from everyone except engine authors forever, and every user familiar with assertions would mistakenly put it in assertion objects by accident from muscle memory.

1 - It's not moot because it's in types, the consequences of rewriting such an "assertion" still has the confusing impact I mentioned in #48644 (comment).

2 - I think a normal outlook here is that if you have a new feature, and that feature might need a new syntax, there's an existing syntax you're eyeing but it has a big incompatibility, you just have to add the new syntax. That's sort of what comprises a language in the first place, right? When JS inevitably has actual import modifiers, we know they're not going to shoehorn it into assert {} for AST stability. Why does TS want to?

3 - I think this is adapting to user expectations in a way that exacerbates their cause for confusion in the long term. Adopting a syntax from JS and twisting its meaning in this manner seems like one of the worser things TS can do for the JS ecosystem. I guess technically that design goal only pertains to transpiled code...


Let's say that we added this to the documentation

import type will search for the specified module under all possible resolution algorithms. If the algorithm that successfully found the module is not one specified by the "resolution-mode" assert, then an error is raised

As a TS-only declaration, we're allowed to specify the semantics of what import type ... "specifier" ... means, and adding an assert about which algorithm succeeded seems like a valid use of it. What conceptual violation has occurred?

Well that's not the user understanding you're trying to build on most importantly, not the implementation and not even going to be written in the doc outside of the hypothetical. What's conceptually sensible about it?

@weswigham
Copy link
Member

weswigham commented Apr 13, 2022

Well a violation would occur if a user has imports with different resolution modes specified for the same resolved specifier.

And this is key flaw in your assumption: this is totally incompatible with node resolution (at least when you're like us and interpret imports as both cjs and esm in the same compilation!) and has to be conceeded so long as, like us, you unify cjs and esm resolvers into a single system rather than splintering it like node itself does. (Which has the same appearance, ultimately.)

Our compilation model has all imports as under a single resolver that acknowledges source location and context - in some senses, it's much more robust than what's specified, by necessity.

Put simply: In node the specifier pkg could resolve to multiple different things in the same file, depending on a number of factors. (context packages, containing file extension, syntax containing the specifier, and command line options, to name a few!)

@weswigham
Copy link
Member

weswigham commented Apr 13, 2022

I'm telling you... import assertions were a mistake :(

Oh, absolutely, terrible idea, but the syntax is there for the using now 😂

@dsherret
Copy link
Contributor Author

@weswigham @DanielRosenwasser @RyanCavanaugh thanks for your points and engaging this thread. Reading through the recently linked design meeting notes has also been more enlightening and thanks for outlining all the other ideas you had.

I can't think of any more suggestions...

@dsherret
Copy link
Contributor Author

Closing due to #49055. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants