GeezerVison

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.

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.

Nit: remove extra space

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:

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.

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.

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