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 support for import defer proposal #60757

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nicolo-ribaudo
Copy link

@nicolo-ribaudo nicolo-ribaudo commented Dec 13, 2024

This proposal is currently at Stage 2.7: the last changes have been approved at the TC39 meeting last week, and I'm planning on proposing Stage 3 at the TC39 meeting on February 18th (the only blocker is me finishing writing test262 tests, and they will for sure be ready by then). I'm already opening this PR because, depending on when TS5.8 will be released, it's possible that the proposal will already be at Stage 3 by then :)

This PR only needs to add parsing support for the proposal:

  • it does not support downleveling it, as it's not possible to transpile it to older ESM versions
    • it would be possible add support for transpiling to CommonJS, if needed.
  • it does not introduce any type-checking changes, as the object created by import defer * as ns is meant to "look like" the one created by import * as ns

As you'll see from the code, whether a module is deferred or not is not a boolean but an enum. That's because of the import source proposal: there is no PR for it yet, but once it will be implemented it should be done by adding one more possible "import phase" to this enum. A question I have is if that should be an enum, or just an union of undefined | DeferKeyword (which will one day become undefined | DeferKeyword | SourceKeyword).

Fixes #59391

This patch was originally written by @ryzokuken

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 13, 2024
@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

Co-authored-by: Nicolò Ribaudo <hell@nicr.dev>
@nicolo-ribaudo
Copy link
Author

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

I guess this goes in favor of using undefined | DeferKeyword, so that it avoids a breaking change in the factory method? :)

@@ -4723,11 +4724,12 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode
}

// @api
function createImportClause(isTypeOnly: boolean, name: Identifier | undefined, namedBindings: NamedImportBindings | undefined): ImportClause {
function createImportClause(isTypeOnly: boolean, name: Identifier | undefined, namedBindings: NamedImportBindings | undefined, phase: ImportPhase): ImportClause {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preemptively noting that this is a breaking API change, and would require a deprecation helper in the deprecations project to tack onto our public API something that will set a default for the new parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may also be the case that phase needs to go after isTypeOnly since AST node builder parameters and properties are intended to be in source order.

Copy link
Member

@rbuckton rbuckton Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to mix type imports with defer or source. Something like import type source foo from "foo" doesn't make sense if all source imports will have the same type, and import type defer * as foo from "foo" would essentially be the same as import type * as foo from "foo".

If we consider type, defer, and source to be mutually exclusive, then I would suggest we replace the isTypeOnly parameter with something like importModifier: SyntaxKind.TypeKeyword | SyntaxKind.DeferKeyword | SyntaxKind.SourceKeyword | boolean | undefined and have ImportClause be:

export interface ImportClause extends NamedDeclaration {
    readonly kind: SyntaxKind.ImportClause;
    readonly parent: ImportDeclaration | JSDocImportTag;
    /** @deprecated */
    readonly isTypeOnly: boolean;
    readonly importModifier: SyntaxKind.TypeKeyword | SyntaxKind.DeferKeyword | SyntaxKind.SourceKeyword | undefined;
    readonly name?: Identifier; // Default binding
    readonly namedBindings?: NamedImportBindings;
}

For back-compat purposes, we can set isTypeOnly to true if importModifier is SyntaxKind.TypeKeyword.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue with that scheme is that it's a little weirder to capture multiple modifiers being used in the cases of error recovery.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally going with something very similar to Ron's suggestion, and consider "type" just as another phase. I ended up not doing it because of the AST breaking change, but if just keeping the old property for backwards compat is ok then I'd go for it.

I think the issue with that scheme is that it's a little weirder to capture multiple modifiers being used in the cases of error recovery.

Error recovery is going to be tricky anyway, because each modifier is a valid identifier so things like import type defer are a potentially valid start of an import declaration. I wonder how likely it is for people to accidentally write two modifiers in an import.

@jakebailey
Copy link
Member

I guess this goes in favor of using undefined | DeferKeyword, so that it avoids a breaking change in the factory method? :)

I'm not an expert per se, but I would expect that these need to be modifiers so that they are nodes that can be walked, since people can stick comments between them and so on...

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 13, 2024

I would expect that these need to be modifiers so that they are nodes that can be walked, since people can stick comments between them and so on...

Modifiers would precede the import here, so it's a bit more iffy than that. But I guess the export @decorator class C {} thing has already made things iffy between decorators/modifiers, and @rbuckton recalls how we handled this.

But this is close syntactically to import type, which just gets an isTypeOnly property slapped on, and hasn't caused issues. When we do need to hydrate nodes in the language service, we'll end up with a synthesized defer node. So from a technical standpoint, I think it's doesn't cause issues, but it might not be the best representation long-term.

@rbuckton
Copy link
Member

rbuckton commented Dec 13, 2024

But this is close syntactically to import type, which just gets an isTypeOnly property slapped on, and hasn't caused issues.

We have emitTokenWithComment to handle synthetic punctuation and comments, so that's fine. IMO we should just use SyntaxKind instead of a separate enum, as I suggested above.

@rbuckton
Copy link
Member

rbuckton commented Dec 13, 2024

That said, ImportClause is parsed after the import keyword, so it could have modifiers and we could just make type a modifier with isTypeOnly existing purely for back-compat purposes. That would require a lot of additional complexity in how we handle modifiers, however, as we'd have to have grammar checks to disallow type, defer, and source as modifiers for most elements (though we do this for accessor, in, out, etc, so it's nothing new).

src/compiler/emitter.ts Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Author

I updated the PR to go with the suggestion in #60757 (comment), which is what I found the cleanest. Happy to do differently if needed. The API change is now backwards compatible. Probably I should do the same change for exports? Even though export defer and export source don't exist, so it's only for type.

I also added a test showing that comments are properly preserved.

emitTokenWithComment(node.phaseModifier, node.pos, writeKeyword, node);
writeSpace();
}
else if (node.isTypeOnly) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this in case somebody passes in an object from an old TS version. Is this relevant for TS' API contract, or would removing it not be considered a breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support mixing TS versions between a given Node and the emitter, so that shouldn't be a concern. Inside the compiler we should avoid depending on deprecated functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

import defer Stage 2.7 proposal support
7 participants