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 importKind to ImportSpecifier (import {type MyType} from './types') #725

Closed
wants to merge 1 commit into from

Conversation

IanVS
Copy link

@IanVS IanVS commented Mar 15, 2022

I'm working on a codemod to transform type imports into the new Typescript 4.5 syntax: facebook/jscodeshift#481

I wanted to use ImportSpecifierBuilder to construct an import specifier that is a type import, but that was impossible. I've never worked with ast-types before, so I'm not sure if I did this correctly. And I was looking for tests to expand, but I couldn't find any import type style tests, so I'm not sure where it should go or what it would look like. Any guidance would be much appreciated!

@IanVS
Copy link
Author

IanVS commented May 21, 2022

Related: #745

Copy link

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @IanVS! I think both this PR and #745 are needed:

One comment below where I think a line should be tweaked.

.field("imported", def("Identifier"));
.build("imported", "local", "importKind")
.field("imported", def("Identifier"))
.field("importKind", or("type", null), defaults["null"]);
Copy link

Choose a reason for hiding this comment

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

Suggested change
.field("importKind", or("type", null), defaults["null"]);
.field("importKind", or("value", "type"), () => "value");

This way it matches the convention found on ImportDeclaration below.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I'd make the change, but as far as I can tell this project is dead. @benjamn doesn't work at Facebook anymore, and I guess nobody else has stepped in to maintain it.

Copy link

Choose a reason for hiding this comment

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

Fair enough. In a future where someone does step up to merge PRs here again, this PR thread can serve as a record of a change that should be made.

I think it's too soon to call the project dead; dormant, yes. Ben merged and wrote a burst of changes as recently as last November.

There's a recent thread over on the related project Recast, where he titled the issue "Recast needs additional maintainers": benjamn/recast#1086 (comment) I expect that if there do arise additional maintainers for Recast, they'll also end up maintaining ast-types, given that they're closely related and ast-types is smaller.

@IanVS IanVS closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants