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

Support import() in rewriteImportExtensions #16794

Merged
merged 4 commits into from
Oct 23, 2024
+76 −27
Merged

Support import() in rewriteImportExtensions #16794

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f20809c
import()
liuxingbaoyu Sep 2, 2024
d1cd129
review
liuxingbaoyu Sep 3, 2024
a0008c7
update
liuxingbaoyu Sep 3, 2024
052fe33
review
liuxingbaoyu Sep 3, 2024
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
28 changes: 20 additions & 8 deletions packages/babel-helpers/src/helpers-generated.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,6 +1030,18 @@ const helpers: Record<string, Helper> = {
dependencies: {},
},
),
// size: 112, gzip size: 112
replaceTsImportExt: helper(
"7.25.7",
'function replaceTsImportExt(e){return/[\\\\/]/.test(e)?e.replace(/(\\.[mc]?)ts$/,"$1js").replace(/\\.tsx$/,".js"):e}',
{
globals: [],
locals: { replaceTsImportExt: ["body.0.id"] },
exportBindingAssignments: [],
exportName: "replaceTsImportExt",
dependencies: {},
},
),
// size: 494, gzip size: 274
set: helper(
"7.0.0-beta.0",
Expand Down Expand Up @@ -1134,15 +1146,15 @@ const helpers: Record<string, Helper> = {
},
},
),
// size: 153, gzip size: 136
// size: 149, gzip size: 134
superPropGet: helper(
"7.25.0",
'function _superPropertyGet(t,e,o,r){var p=get(getPrototypeOf(1&r?t.prototype:t),e,o);return 2&r&&"function"==typeof p?function(t){return p.apply(o,t)}:p}',
'function _superPropGet(t,o,e,r){var p=get(getPrototypeOf(1&r?t.prototype:t),o,e);return 2&r&&"function"==typeof p?function(t){return p.apply(e,t)}:p}',
{
globals: [],
locals: { _superPropertyGet: ["body.0.id"] },
locals: { _superPropGet: ["body.0.id"] },
exportBindingAssignments: [],
exportName: "_superPropertyGet",
exportName: "_superPropGet",
dependencies: {
get: ["body.0.body.body.0.declarations.0.init.callee"],
getPrototypeOf: [
Expand All @@ -1151,15 +1163,15 @@ const helpers: Record<string, Helper> = {
},
},
),
// size: 92, gzip size: 98
// size: 88, gzip size: 95
superPropSet: helper(
"7.25.0",
"function _superPropertySet(t,e,o,r,p,f){return set(getPrototypeOf(f?t.prototype:t),e,o,r,p)}",
"function _superPropSet(t,e,o,r,p,f){return set(getPrototypeOf(f?t.prototype:t),e,o,r,p)}",
{
globals: [],
locals: { _superPropertySet: ["body.0.id"] },
locals: { _superPropSet: ["body.0.id"] },
exportBindingAssignments: [],
exportName: "_superPropertySet",
exportName: "_superPropSet",
dependencies: {
set: ["body.0.body.body.0.argument.callee"],
getPrototypeOf: ["body.0.body.body.0.argument.arguments.0.callee"],
Expand Down
7 changes: 7 additions & 0 deletions packages/babel-helpers/src/helpers/replaceTsImportExt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/* @minVersion 7.25.7 */

export default function replaceTsImportExt(source: string) {
return /[\\/]/.test(source)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, it is not sufficient to detect the bare import; consider @foo/bar.ts .

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That might need to be rewritten, if for example you are remapping

<script type="importmap">
  {
    "imports": {
      "@components/": "./ui/components"
    }
  }
</script>

We added the exception for "it needs to have a slash" because:

  • there are known npm packages with no scope that end in .ts
  • if you use import maps and have no slash, it means you are explicitly writing the import specifier in your import map

So far nobody ever reported a problem with the @foo/bar.ts handling, but if there is a package out there needing it the only way forward is to provide a function option in the config once somebody needs it.

? source.replace(/(\.[mc]?)ts$/, "$1js").replace(/\.tsx$/, ".js")
: source;
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This could probably be shorter:

return String(source).replace(/([\\/].*\.[mc]?)tsx?$/, "$1js");

and it becomes short enough that we could even just inline it in the code without a helper, so we don't have the problem of it not being available.

}
2 changes: 1 addition & 1 deletion packages/babel-helpers/src/helpers/superPropGet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const enum Flags {
Call = 0b10,
}

export default function _superPropertyGet(
export default function _superPropGet(
classArg: any,
property: string,
receiver: any,
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-helpers/src/helpers/superPropSet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import set from "./set.ts";
import getPrototypeOf from "./getPrototypeOf.ts";

export default function _superPropertySet(
export default function _superPropSet(
classArg: any,
property: string,
value: any,
Expand Down
51 changes: 41 additions & 10 deletions packages/babel-preset-typescript/src/plugin-rewrite-ts-imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,55 @@ import { declare } from "@babel/helper-plugin-utils";
import type { types as t, NodePath } from "@babel/core";

export default declare(function ({ types: t }) {
function maybeReplace(
source: t.ArgumentPlaceholder | t.SpreadElement | t.Expression,
path: NodePath,
) {
if (!source) return;
if (t.isStringLiteral(source)) {
if (/[\\/]/.test(source.value)) {
source.value = source.value
.replace(/(\.[mc]?)ts$/, "$1js")
.replace(/\.tsx$/, ".js");
}
return;
}

try {
path.replaceWith(
t.callExpression(path.hub.addHelper("replaceTsImportExt"), [source]),
);
} catch {
// replaceTsImportExt may not be available
}
}

return {
name: "preset-typescript/plugin-rewrite-ts-imports",
visitor: {
"ImportDeclaration|ExportAllDeclaration|ExportNamedDeclaration"({
node,
}: NodePath<
t.ImportDeclaration | t.ExportAllDeclaration | t.ExportNamedDeclaration
>) {
const { source } = node;
"ImportDeclaration|ExportAllDeclaration|ExportNamedDeclaration"(
path: NodePath<
| t.ImportDeclaration
| t.ExportAllDeclaration
| t.ExportNamedDeclaration
>,
) {
const node = path.node;
const kind = t.isImportDeclaration(node)
? node.importKind
: node.exportKind;
if (kind === "value" && source && /[\\/]/.test(source.value)) {
source.value = source.value
.replace(/(\.[mc]?)ts$/, "$1js")
.replace(/\.tsx$/, ".js");
if (kind === "value") {
maybeReplace(node.source, path.get("source"));
}
},
CallExpression(path) {
if (t.isImport(path.node.callee)) {
maybeReplace(path.node.arguments[0], path.get("arguments.0"));
}
},
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
ImportExpression(path) {
maybeReplace(path.node.source, path.get("source"));
},
},
};
});
2 changes: 2 additions & 0 deletions ...et-typescript/test/fixtures/opts/rewriteImportExtensions-createImportExpressions/input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import("./a.ts");
import(a);
7 changes: 7 additions & 0 deletions ...ypescript/test/fixtures/opts/rewriteImportExtensions-createImportExpressions/options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"sourceType": "module",
"presets": [["typescript", { "rewriteImportExtensions": true }]],
"parserOpts": {
"createImportExpressions": true
}
}
2 changes: 2 additions & 0 deletions ...-typescript/test/fixtures/opts/rewriteImportExtensions-createImportExpressions/output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import("./a.js");
import(babelHelpers.replaceTsImportExt(a));
Copy link
Member

Choose a reason for hiding this comment

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

a might not be a string -- in that case we need to do String(a) first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a + "" should align to the ToString AO better than String(), since String(symbol) will return its description while ToString(symbol) is a type error.

3 changes: 3 additions & 0 deletions packages/babel-preset-typescript/test/fixtures/opts/rewriteImportExtensions/input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,6 @@ import "./react.ctsx";
import "a-package/file.ts";
// Bare import, it's either a node package or remapped by an import map
import "soundcloud.ts";

import("./a.ts");
import(a);
2 changes: 2 additions & 0 deletions packages/babel-preset-typescript/test/fixtures/opts/rewriteImportExtensions/output.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ import "./react.ctsx";
import "a-package/file.js";
// Bare import, it's either a node package or remapped by an import map
import "soundcloud.ts";
import("./a.js");
import(babelHelpers.replaceTsImportExt(a));
Loading