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

FUTURE: remove import assertions support for JavaScript #23541

Conversation

petamoriken
Copy link
Contributor

@petamoriken petamoriken commented Apr 24, 2024

Ref #17944, swc-project/swc#8893

TypeScript removes the assert keywords in the transpile, so this PR only works for JavaScript files

@petamoriken petamoriken force-pushed the feat/deny-import-assertions-with-deno-future-flag branch from b14329e to 6b636a5 Compare April 24, 2024 18:46
cli/main.rs Outdated Show resolved Hide resolved
cli/args/mod.rs Outdated Show resolved Hide resolved
@petamoriken petamoriken force-pushed the feat/deny-import-assertions-with-deno-future-flag branch from f39ec24 to a23a992 Compare April 27, 2024 04:49
@petamoriken petamoriken marked this pull request as ready for review April 27, 2024 04:52
@petamoriken petamoriken force-pushed the feat/deny-import-assertions-with-deno-future-flag branch from a23a992 to e330a91 Compare April 27, 2024 05:05
@petamoriken petamoriken force-pushed the feat/deny-import-assertions-with-deno-future-flag branch from e330a91 to 830b68b Compare April 27, 2024 05:06
@petamoriken petamoriken changed the title FUTURE: remove import assertions support FUTURE: remove import assertions support for JavaScript Apr 27, 2024
_ => {
if *DENO_FUTURE {
// deno_ast removes TypeScript `assert` keywords, so this flag only affects JavaScript
// TODO(petamoriken): Need to check TypeScript `assert` keywords in deno_ast
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should address this one before landing the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so. It would be better to work on a separate PR for TypeScript, since it is likely that the error will be displayed on the deno_ast side (swc-project/swc#8893 (comment)), rather than using this flag.

Copy link
Member

Choose a reason for hiding this comment

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

Let me talk with @dsherret about this first 👍

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 maybe we should just not worry about it for now? It would be a lot of work and supporting it can make more code just keep working for the time being.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@bartlomieju bartlomieju merged commit 783533d into denoland:main Apr 29, 2024
17 checks passed
@petamoriken petamoriken deleted the feat/deny-import-assertions-with-deno-future-flag branch April 30, 2024 00:28
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.

None yet

3 participants