Import assertion #40698

Kingwl · 2020-09-22T10:44:08Z

Kingwl · 2020-09-22T16:59:28Z

typescript-bot · 2020-09-22T16:59:49Z

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 375b433. You can monitor the build here.

Jack-Works · 2020-09-23T00:57:13Z

src/compiler/checker.ts

+        function checkGrammarImportAssertion(declaration: ImportDeclaration | ExportDeclaration) {
+            const target = getEmitScriptTarget(compilerOptions);
+            if (target < ScriptTarget.ESNext && declaration.assertClause) {
+                grammarErrorOnNode(declaration.assertClause, Diagnostics.Import_assertions_are_not_available_when_targeting_lower_than_esnext);


According to the current specification of Import Assertion, the import clause cannot affect the representation of the source text. Does it mean it can be safely ignored (when downlevel, drop the whole assert clause instead of emitting an error)?

Well. IMO, downlevel means something could map to another. Ignore is not a good idea.

But maybe we could control the error by module options rather than target.

Import Assertions are an enrichment to the import statement, provides the runtime with extra information of which it can make a decision about. I could see the need to allow an error by default, but allow the error to be ignored and the node to be dropped from the emit.

The biggest use case is supporting JSON modules, and there are several runtimes that already support JSON modules without the assertion, but I can see people wanting to start writing the assertions in the code for the runtimes that will only support it with the assertion clause.

typescript-bot · 2020-11-12T10:29:39Z

The TypeScript team hasn't accepted the linked issue #40694. If you can get it accepted, this PR will have a better chance of being reviewed.

Kingwl · 2020-12-31T07:30:57Z

uP.

src/compiler/checker.ts

Kingwl · 2021-09-08T03:28:03Z

Up.

rbuckton · 2021-09-09T19:20:51Z

Sorry for the delay @Kingwl. I'm looking at this again today.

rbuckton

FYI, I will be OOF through Tuesday and may not be able to respond to feedback until Wednesday.

src/compiler/checker.ts

src/compiler/emitter.ts

src/compiler/utilitiesPublic.ts

src/compiler/types.ts

rbuckton · 2021-09-09T20:26:23Z

src/compiler/checker.ts

            // see: parseArgumentOrArrayLiteralElement...we use this function which parse arguments of callExpression to parse specifier for dynamic import.
            // parseArgumentOrArrayLiteralElement allows spread element to be in an argument list which is not allowed as specifier in dynamic import.
-            if (isSpreadElement(nodeArguments[0])) {
+            if (nodeArguments.length && isSpreadElement(nodeArguments[0])) {


Do we need to check that all elements of the assert property of the second argument have type string? Per https://tc39.es/proposal-import-assertions/#sec-evaluate-import-call, Step 10.d.v.3:

No coercion to string is performed here, the algorithm explicitly checks for strings.

We might want to consider something like the ImportMeta interface for the import argument, something like:

// es5.d.ts interface ImportCallOptions { assert?: ImportAssertions; } interface ImportAssertions { type?: string; [key: string]: string | undefined; // not ideal, but necessary so that `type` is optional. }

Then we can check against the global ImportCallOptions type, and if other assertions or options are added later, they can be introduced using global augmentation similar to ImportMeta.

We could consider this as a follow-on PR if necessary, but it would be nice to have.

NOTE: if we wanted to be more specific with type we'd need to create an open-ended union via an interface as well:

interface ImportAssertions { type?: ImportTypeAssertion; [key: string]: string | undefined; } // see ArrayBufferTypes for reference interface ImportTypeAssertionTypes { json: "json"; } type ImportTypeAssertion = Extract<ImportTypeAssertions[keyof ImportTypeAssertions], string>;

That way, we have better completions for type and if a host environment supported other values for type, they could also be added via augmentation, i.e.:

interface ImportTypeAssertionTypes { css: "css"; }

Also, the nodeArguments.length check shouldn't be necessary if checkGrammarImportCallArguments already asserts that nodeArguments.length can't be zero.

We also need to ensure that nodeArguments[1] (if present) is not a spread element.

I would suggest this approach:

const spreadElement = forEach(nodeArguments, isSpreadElement); if (spreadElement) { return grammarErrorOnNode(spreadElement, Diagnostics.Specifier_of_dynamic_import_cannot_be_spread_element); }

That way it checks all arguments, but only reports on the first error (which is almost always the case for grammar errors).

Should we check the assert clause too？Or just the second parameter of dynamic import.

tests/baselines/reference/importAssertion1(module=esnext).errors.txt

src/compiler/checker.ts

rbuckton

A few more minor changes. @DanielRosenwasser, @andrewbranch do we want to add something like #40698 (comment) in this PR or in a follow up?

rbuckton · 2021-09-15T23:32:06Z

src/compiler/checker.ts


-            if (node.typeArguments) {
-                return grammarErrorOnNode(node, Diagnostics.Dynamic_import_cannot_have_type_arguments);
+            if (nodeArguments.length  === 0 || nodeArguments.length > 2) {


Nit: remove extra space

Suggested change

if (nodeArguments.length === 0 || nodeArguments.length > 2) {

if (nodeArguments.length === 0 || nodeArguments.length > 2) {

rbuckton · 2021-09-15T23:35:02Z

src/compiler/checker.ts

+            const spreadElement = forEach(nodeArguments, isSpreadElement);
+            if (spreadElement) {
                return grammarErrorOnNode(nodeArguments[0], Diagnostics.Specifier_of_dynamic_import_cannot_be_spread_element);
            }


Hmm. This doesn't quite do what we want. We want to report the error on the spread element, not the first argument (which might not be the spread element). I think we want this instead:

Suggested change

const spreadElement = forEach(nodeArguments, isSpreadElement);

if (spreadElement) {

return grammarErrorOnNode(nodeArguments[0], Diagnostics.Specifier_of_dynamic_import_cannot_be_spread_element);

}

const spreadElement = find(nodeArguments, isSpreadElement);

if (spreadElement) {

return grammarErrorOnNode(spreadElement, Diagnostics.Specifier_of_dynamic_import_cannot_be_spread_element);

}

rbuckton · 2021-09-15T23:40:26Z

src/compiler/types.ts

-        createImportDeclaration(decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, importClause: ImportClause | undefined, moduleSpecifier: Expression, assertClause: AssertClause | undefined): ImportDeclaration;
-        updateImportDeclaration(node: ImportDeclaration, decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, importClause: ImportClause | undefined, moduleSpecifier: Expression, assertClause: AssertClause | undefined): ImportDeclaration;
+        createImportDeclaration(decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, importClause: ImportClause | undefined, moduleSpecifier: Expression, assertClause?: AssertClause): ImportDeclaration;
+        updateImportDeclaration(node: ImportDeclaration, decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, importClause: ImportClause | undefined, moduleSpecifier: Expression, assertClause?: AssertClause): ImportDeclaration;


I would leave | undefined here instead of adding ?. My previous comment was specific to the create. For a create, there's no sense breaking existing code because they weren't previously passing an argument. For an update, we do want to break existing code because there's now a new node they may not be appropriately handling.

rbuckton · 2021-09-15T23:42:17Z

src/compiler/types.ts

@@ -7360,7 +7360,7 @@ namespace ts {
        createExportAssignment(decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, isExportEquals: boolean | undefined, expression: Expression): ExportAssignment;
        updateExportAssignment(node: ExportAssignment, decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, expression: Expression): ExportAssignment;
        createExportDeclaration(decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, isTypeOnly: boolean, exportClause: NamedExportBindings | undefined, moduleSpecifier?: Expression, assertClause?: AssertClause): ExportDeclaration;
-        updateExportDeclaration(node: ExportDeclaration, decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, isTypeOnly: boolean, exportClause: NamedExportBindings | undefined, moduleSpecifier: Expression | undefined, assertClause: AssertClause | undefined): ExportDeclaration;
+        updateExportDeclaration(node: ExportDeclaration, decorators: readonly Decorator[] | undefined, modifiers: readonly Modifier[] | undefined, isTypeOnly: boolean, exportClause: NamedExportBindings | undefined, moduleSpecifier: Expression | undefined, assertClause?: AssertClause): ExportDeclaration;


I also would leave | undefined here. We want to break callers here since they could be forgetting to handle a node they weren't previously handling.

DanielRosenwasser · 2021-09-16T00:13:04Z

src/compiler/diagnosticMessages.json

@@ -1388,6 +1388,10 @@
        "category": "Message",
        "code": 1449
    },
+    "Dynamic import must only have a specifier and an optional assertion as arguments": {


Suggested change

"Dynamic import must only have a specifier and an optional assertion as arguments": {

"Dynamic imports can only accept a path and an optional assertion as arguments": {

Agree, but I think module specifier is a more correct term than path

DanielRosenwasser · 2021-09-16T00:13:19Z

src/compiler/diagnosticMessages.json

@@ -924,7 +924,7 @@
        "category": "Error",
        "code": 1323
    },
-    "Dynamic import must have one specifier as an argument.": {
+    "Dynamic import only supports a second argument when the '--module' option is set to 'esnext'.": {


Suggested change

"Dynamic import only supports a second argument when the '--module' option is set to 'esnext'.": {

"Dynamic imports only support a second argument when the '--module' option is set to 'esnext'.": {

andrewbranch · 2021-09-16T00:13:22Z

Do we need to check that all elements of the assert property of the second argument have type string?

Let’s try to get this in this PR if possible. Preventing runtime errors seems high priority to me. I personally like the idea of an open-ended interface, but I think that warrants some design discussion to make sure we’re all on the same page. Let’s discuss at Friday’s meeting.

rbuckton · 2021-09-17T19:41:27Z

@Kingwl, if you don't mind I'm going to make some of these minor changes and add a basic implementation of #40698 (comment) that we can expand on later.

rbuckton · 2021-09-17T22:00:46Z

I found a small bug in program.ts related to import("./module", {}) in that we aren't resolving ./module because the argument count isn't 1. I'll push changes up shortly. It is definitely strange to not get completions at | in import("./module", { | }), and not getting a signature help popup.

# Conflicts: # src/compiler/diagnosticMessages.json

Kingwl · 2021-09-18T05:00:51Z

Never mind. Thanks for your changes.

rbuckton · 2021-09-20T20:35:35Z

I fixed completions for import("path", { | }), but we still don't handle completions in import ... assert { }. This is probably fine, since we've typed ImportAssertions to just [key: string]: string so you wouldn't really get useful completions right now anyways. If we decide to make type a property of ImportAssertions in a later PR, we'll want to re-investigate completions for import/export with assert.

rbuckton · 2021-09-20T21:15:57Z

Thanks for all the effort you put into this @Kingwl!

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 22, 2020

Add parsing

e1b3b78

Kingwl force-pushed the import_assertion branch from f945374 to e1b3b78 Compare September 22, 2020 10:44

Kingwl added 5 commits September 22, 2020 22:00

fix all api

66d2273

check gramma of import call

5f3cea6

Add more part of assertion

5a1af04

Add some case

4b51ca9

Add baseline

375b433

Kingwl marked this pull request as ready for review September 22, 2020 16:59

Jack-Works reviewed Sep 23, 2020

View reviewed changes

kitsonk mentioned this pull request Sep 23, 2020

Import Assertions/JSON Modules denoland/deno#7623

Closed

use module insted of target

41a881c

typescript-bot assigned rbuckton Sep 23, 2020

typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 23, 2020

Kingwl added 2 commits September 23, 2020 21:56

strip assertion in d.ts

46a3eb1

Merge branch 'master' into import_assertion

a484ea2

sandersn added this to Not started in PR Backlog Oct 2, 2020

sandersn moved this from Not started to Needs review in PR Backlog Oct 5, 2020

Merge branch 'master' into import_assertion

d64f7ba

Merge branch 'master' into import_assertion

68c8c5d

Kingwl added 4 commits January 7, 2021 17:39

Merge branch 'master' into import_assertion

29fe0d3

Update new baseline

adcfd1b

Merge branch 'master' into import_assertion

fb01eb3

accept baseline

4f22a60

Jack-Works reviewed Mar 8, 2021

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

Add some comment

106ff06

rbuckton requested changes Sep 9, 2021

View reviewed changes

rbuckton reviewed Sep 9, 2021

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

justinfagnani mentioned this pull request Sep 13, 2021

Implement Import Assertions (stage 3) #40694

Closed

5 tasks

Kingwl added 3 commits September 14, 2021 19:36

Merge branch 'main' into import_assertion

7df4964

Fix cr issues

6ea3c55

Fix more issue

fc51c42

IanVS mentioned this pull request Sep 15, 2021

Support Import Assertions vitejs/vite#4934

Closed

rbuckton requested changes Sep 15, 2021

View reviewed changes

DanielRosenwasser reviewed Sep 16, 2021

View reviewed changes

DanielRosenwasser mentioned this pull request Sep 17, 2021

Design Meeting Notes, 9/17/2021 #45944

Closed

rbuckton added 2 commits September 17, 2021 15:20

Incorporate PR feedback, fix module resolution for import()

bf7ca71

Merge branch 'main' into import_assertion

463138a

# Conflicts: # src/compiler/diagnosticMessages.json

rbuckton approved these changes Sep 20, 2021

View reviewed changes

PR Backlog automation moved this from Waiting on author to Needs merge Sep 20, 2021

Add contextual type and completions for ImportCall options argument

b07c6e9

rbuckton force-pushed the import_assertion branch from 9accee9 to b07c6e9 Compare September 20, 2021 21:14

rbuckton merged commit ec114b8 into microsoft:main Sep 20, 2021

PR Backlog automation moved this from Needs merge to Done Sep 20, 2021

dnalborczyk mentioned this pull request Nov 7, 2021

feat: add support for import assertions rollup/rollup#4267

Closed

9 tasks

KSXGitHub mentioned this pull request Oct 23, 2022

Caching and loading non-importable resources denoland/deno#5987

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import assertion #40698

Import assertion #40698

Kingwl commented Sep 22, 2020

Kingwl commented Sep 22, 2020

typescript-bot commented Sep 22, 2020 •

edited

Jack-Works Sep 23, 2020

Kingwl Sep 23, 2020

kitsonk Sep 23, 2020

typescript-bot commented Nov 12, 2020

Kingwl commented Dec 31, 2020

Kingwl commented Sep 8, 2021

rbuckton commented Sep 9, 2021

rbuckton left a comment

rbuckton Sep 9, 2021

rbuckton Sep 9, 2021

rbuckton Sep 9, 2021

rbuckton Sep 9, 2021

Kingwl Sep 16, 2021

rbuckton left a comment

rbuckton Sep 15, 2021

rbuckton Sep 15, 2021

rbuckton Sep 15, 2021

rbuckton Sep 15, 2021

DanielRosenwasser Sep 16, 2021

rbuckton Sep 17, 2021

DanielRosenwasser Sep 16, 2021

andrewbranch commented Sep 16, 2021

rbuckton commented Sep 17, 2021

rbuckton commented Sep 17, 2021

Kingwl commented Sep 18, 2021

rbuckton commented Sep 20, 2021

rbuckton commented Sep 20, 2021

	if (nodeArguments.length === 0 \|\| nodeArguments.length > 2) {
	if (nodeArguments.length === 0 \|\| nodeArguments.length > 2) {

	"Dynamic import must only have a specifier and an optional assertion as arguments": {
	"Dynamic imports can only accept a path and an optional assertion as arguments": {

	"Dynamic import only supports a second argument when the '--module' option is set to 'esnext'.": {
	"Dynamic imports only support a second argument when the '--module' option is set to 'esnext'.": {

Import assertion #40698

Import assertion #40698

Conversation

Kingwl commented Sep 22, 2020

Kingwl commented Sep 22, 2020

typescript-bot commented Sep 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Nov 12, 2020

Kingwl commented Dec 31, 2020

Kingwl commented Sep 8, 2021

rbuckton commented Sep 9, 2021

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewbranch commented Sep 16, 2021

rbuckton commented Sep 17, 2021

rbuckton commented Sep 17, 2021

Kingwl commented Sep 18, 2021

rbuckton commented Sep 20, 2021

rbuckton commented Sep 20, 2021

typescript-bot commented Sep 22, 2020 •

edited