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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 11, 2025
Merged

Fix ExperimentalWarning about importAssertions #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89217db
Fix `ExperimentalWarning` about `importAssertions`
danielfinke Jan 31, 2025
2e0508d
Merge branch 'main' into use-importAttributes-instead-of-importAssert…
laverdet Feb 11, 2025
44ca462
Version tweak
laverdet Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions loader/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ function extractImportAttributes(params: URLSearchParams): ImportAttributes {
return Object.fromEntries(entries);
}

const deprecatedAssertSyntax = /^1[6-9]/.test(process.versions.node);
// 16.14.0 >= version < 18.20.0
const deprecatedAssertSyntax = /^(?:16|17|18\.1?[0-9]\.)/.test(process.versions.node);

const makeAdapterModule = (url: string, importAttributes: ImportAttributes) => {
const encodedURL = JSON.stringify(url);
Expand Down Expand Up @@ -128,21 +129,20 @@ export const resolve: ResolveHook = (specifier, context, nextResolve) => {
const resolutionURL = new URL(specifier);
const resolutionSpecifier = resolutionURL.searchParams.get("specifier");
assert.ok(resolutionSpecifier !== null);
const importAssertions = extractImportAttributes(resolutionURL.searchParams);
const importAttributes = extractImportAttributes(resolutionURL.searchParams);
return maybeThen(function*() {
const result: ResolveFnOutput = yield nextResolve(resolutionSpecifier, {
...context,
parentURL: parentModuleURL,
// @ts-expect-error -- remove after nodejs v18(?) is not supported
importAssertions,
[deprecatedAssertSyntax ? "importAssertions" : "importAttributes"]: importAttributes,
});
const params = new URLSearchParams([
[ "url", result.url ],
...Fn.filter(resolutionURL.searchParams, entry => entry[0] === "with"),
]);
return {
...result,
importAssertions: {},
[deprecatedAssertSyntax ? "importAssertions" : "importAttributes"]: {},
url: `hot:module?${String(params)}`,
};
});
Expand All @@ -154,21 +154,20 @@ export const resolve: ResolveHook = (specifier, context, nextResolve) => {
assert.ok(resolutionSpecifier !== null);
const parentModuleURL = resolutionURL.searchParams.get("parent");
assert.ok(parentModuleURL !== null);
const importAssertions = extractImportAttributes(resolutionURL.searchParams);
const importAttributes = extractImportAttributes(resolutionURL.searchParams);
return maybeThen(function*() {
const result: ResolveFnOutput = yield nextResolve(resolutionSpecifier, {
...context,
parentURL: parentModuleURL,
// @ts-expect-error -- remove after nodejs v18(?) is not supported
importAssertions,
[deprecatedAssertSyntax ? "importAssertions" : "importAttributes"]: importAttributes,
});
const params = new URLSearchParams([
[ "url", result.url ],
...Fn.filter(resolutionURL.searchParams, entry => entry[0] === "with"),
]);
return {
...result,
importAssertions: {},
[deprecatedAssertSyntax ? "importAssertions" : "importAttributes"]: {},
url: `hot:module?${String(params)}`,
};
});
Expand Down Expand Up @@ -222,13 +221,13 @@ export const load: LoadHook = (urlString, context, nextLoad) => {
const url = new URL(urlString);
switch (url.pathname) {
case "adapter": {
const importAssertions = extractImportAttributes(url.searchParams);
const importAttributes = extractImportAttributes(url.searchParams);
const moduleURL = url.searchParams.get("url");
assert.ok(moduleURL !== null);
return {
shortCircuit: true,
format: "module",
source: makeAdapterModule(moduleURL, importAssertions),
source: makeAdapterModule(moduleURL, importAttributes),
};
}

Expand All @@ -252,11 +251,8 @@ export const load: LoadHook = (urlString, context, nextLoad) => {
const hot = new LoaderHot(moduleURL, port);
const result = await nextLoad(moduleURL, {
...context,
// TODO [marcel 2024-04-27]: remove after nodejs v18(?) is not supported
// @ts-expect-error -- remove after nodejs v18(?) is not supported
importAssertions: importAttributes,
importAttributes,
hot,
[deprecatedAssertSyntax ? "importAssertions" : "importAttributes"]: importAttributes,
});
if (!ignorePattern.test(moduleURL)) {
if (result.format === "module") {
Expand Down
5 changes: 3 additions & 2 deletions loader/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ interface ImportEntry {
bindings: BindingEntry[];
}

const deprecatedAssertSyntax = /^1[6-9]/.test(process.versions.node);
// version < v22
const deprecatedAssertSyntax = /^(?:16|17|18|19|20|21)/.test(process.versions.node);

export function transformModuleSource(
filename: string,
Expand All @@ -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.

],
},
});
Expand Down