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

Fix ExperimentalWarning about importAssertions #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danielfinke
Copy link
Contributor

Use importAttributes instead of importAssertions if running on a Node.js version >= 18.20.0. On lower versions, continue to use importAssertions.

Left the comment re: removal of the deprecated option - can still remove Node.js 18 is no longer supported.

closes #7

Use `importAttributes` instead of `importAssertions` if running on a
Node.js version >= 18.20.0. On lower versions, continue to use
`importAssertions`.

Left the comment re: removal of the deprecated option - can still remove
Node.js 18 is no longer supported.

closes braidnetworks#7
Copy link
Contributor

@laverdet laverdet left a comment

Choose a reason for hiding this comment

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

Thanks. This is a good continuation of the work from this commit 4b61a60 it looks like I missed some spots. I think a tweak to one of the expressions would make this more correct, let me know what you think.

@@ -41,7 +42,7 @@ export function transformModuleSource(
parserOpts: {
plugins: [
"explicitResourceManagement",
[ "importAttributes", { deprecatedAssertSyntax: true } ],
[ "importAttributes", { deprecatedAssertSyntax } ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on the unused expression above.

I don't think this is what we want though. I believe this would prevent legacy asserts syntax from working after nodejs v18.20. v18 & v20 are supposed to be the transitional versions where either syntax works. So it gives teams a chance to migrate.

I think there need to be two distinct regular expressions. The first is the most aggressive/eager, in order to avoid deprecation warnings. That's the one you authored in loader.ts, it looks correct to me. The second one would be the least aggressive option, which should go here. I think it would be /^(?:16|17|18|19|20|21)/. So even if the user is using asserts syntax in nodejs v20 we will still allow it. It will hide the deprecation notice from them which is unfortunate but definitely not worth making our own deprecation notice.

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.

Use importAttributes instead of importAssertions if possible to resolve warning message
2 participants