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

feat: TsImportType - support import attributes #9796

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

Conversation

dsherret
Copy link
Contributor

@dsherret dsherret commented Dec 12, 2024

Description:

This parses, emits, and visits import attributes in TsImportType.

BREAKING CHANGE:

Adds an additional property to TsImportType

Related issue (if exists): #9377

@dsherret dsherret requested a review from a team as a code owner December 12, 2024 16:49
Copy link

changeset-bot bot commented Dec 12, 2024

⚠️ No Changeset found

Latest commit: 5781038

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -546,6 +546,8 @@ pub struct TsImportType {
pub qualifier: Option<TsEntityName>,
#[cfg_attr(feature = "serde-impl", serde(rename = "typeArguments"))]
pub type_args: Option<Box<TsTypeParamInstantiation>>,
#[cfg_attr(feature = "serde-impl", serde(default))]
pub with: Option<Box<ObjectLit>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this name because this is the full { with: { "module-resolution": "import" } } (which I need for dprint-plugin-typescript). Maybe something else would be better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll wait to update the failing tests until getting feedback on this.

Copy link
Member

Choose a reason for hiding this comment

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

So annotation? I think we may need to use separate type for this kind of annotations. How do you think? cc @swc-project/core-es

Copy link
Member

Choose a reason for hiding this comment

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

Is with hardcoded?
I hope there is no situation like { with: { "foo": "bar" }, other: "value" }.

Copy link
Contributor

@CPunisher CPunisher Dec 14, 2024

Choose a reason for hiding this comment

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

From https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html

The expected type of that second argument is defined by a type called ImportCallOptions, which by default just expects a property called with.

And typescript playground throws Object literal may only specify known properties, and 'xxx' does not exist in type 'ImportCallOptions'.

So I think some names related to ImportCallOptions or just attributes are ok as well.

Copy link
Member

@kdy1 kdy1 Dec 14, 2024

Choose a reason for hiding this comment

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

I think we need a breaking change anyway if there's a situation like { with: { "foo": "bar" }, other: "value" }, and I like the idea of

ImportCallOptions or just attributes

@@ -316,6 +316,18 @@ impl<I: Tokens> Parser<I> {
}
};

// the "assert" keyword is deprecated and this syntax is niche, so
// don't support it
Copy link
Contributor Author

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 should support the "assert" keyword for these reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was difficult to figure out how to run (cargo run --package generate-code -- --input-dir crates/swc_ecma_ast --output crates/swc_ecma_visit/src/generated.rs). It might be good to add instructions somewhere.

Also, running that command generates a bunch of other things that I reverted because they were unrelated to this PR. I think maybe this file is out of date on main. It might be good to add some CI verification to ensure it stays up to date.

Copy link

codspeed-hq bot commented Dec 12, 2024

CodSpeed Performance Report

Merging #9796 will improve performances by 3.81%

Comparing dsherret:feat_support_import_attributes_ts_import_type (5781038) with main (e04c7b3)

Summary

⚡ 2 improvements
✅ 192 untouched benchmarks

Benchmarks breakdown

Benchmark main dsherret:feat_support_import_attributes_ts_import_type Change
common/allocator/alloc/cached-no-scope/1000000 112 ms 108.6 ms +3.14%
es/visitor/base-perf/boxing_boxed_clone 2.4 µs 2.3 µs +3.81%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants