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
perf(resolve): avoid isWorkerRequest and clean up .ts imported a .js #12571
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,15 +22,13 @@ import { | |
deepImportRE, | ||
ensureVolumeInPath, | ||
fsPathFromId, | ||
getPotentialTsSrcPaths, | ||
injectQuery, | ||
isBuiltin, | ||
isDataUrl, | ||
isExternalUrl, | ||
isNonDriveRelativeAbsolutePath, | ||
isObject, | ||
isOptimizable, | ||
isPossibleTsOutput, | ||
isTsRequest, | ||
isWindows, | ||
lookupFile, | ||
|
@@ -48,7 +46,6 @@ import { | |
loadPackageData, | ||
resolvePackageData, | ||
} from '../packages' | ||
import { isWorkerRequest } from './worker' | ||
|
||
const normalizedClientEntry = normalizePath(CLIENT_ENTRY) | ||
const normalizedEnvEntry = normalizePath(ENV_ENTRY) | ||
|
@@ -177,16 +174,13 @@ export function resolvePlugin(resolveOptions: InternalResolveOptions): Plugin { | |
} | ||
|
||
if (importer) { | ||
const _importer = isWorkerRequest(importer) | ||
? splitFileAndPostfix(importer).file | ||
: importer | ||
if ( | ||
isTsRequest(_importer) || | ||
isTsRequest(importer) || | ||
resolveOpts.custom?.depScan?.loader?.startsWith('ts') | ||
) { | ||
options.isFromTsImporter = true | ||
} else { | ||
const moduleLang = this.getModuleInfo(_importer)?.meta?.vite?.lang | ||
const moduleLang = this.getModuleInfo(importer)?.meta?.vite?.lang | ||
options.isFromTsImporter = moduleLang && isTsRequest(`.${moduleLang}`) | ||
} | ||
} | ||
|
@@ -531,6 +525,9 @@ function tryFsResolve( | |
if (res) return res + postfix | ||
} | ||
|
||
const knownTsOutputRE = /\.(?:js|mjs|cjs|jsx)$/ | ||
const isPossibleTsOutput = (url: string): boolean => knownTsOutputRE.test(url) | ||
Comment on lines
+528
to
+529
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved this function here because it isn't a widely used util and it only works without query, that is what we need in |
||
|
||
function tryCleanFsResolve( | ||
file: string, | ||
options: InternalResolveOptions, | ||
|
@@ -555,10 +552,21 @@ function tryCleanFsResolve( | |
if (dirStat?.isDirectory()) { | ||
if (possibleJsToTs) { | ||
// try resolve .js, .mjs, .mts or .jsx import to typescript file | ||
patak-dev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const tsSrcPaths = getPotentialTsSrcPaths(file) | ||
for (const srcPath of tsSrcPaths) { | ||
if ((res = tryResolveRealFile(srcPath, preserveSymlinks))) return res | ||
} | ||
const type = path.extname(file) | ||
patak-dev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const fileName = file.slice(0, -type.length) | ||
if ( | ||
(res = tryResolveRealFile( | ||
fileName + type.replace('js', 'ts'), | ||
preserveSymlinks, | ||
)) | ||
) | ||
return res | ||
// for .js, also try .tsx | ||
if ( | ||
type === '.js' && | ||
(res = tryResolveRealFile(fileName + '.tsx', preserveSymlinks)) | ||
) | ||
return res | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It was also doing a complex regex taking into account query params. This function was hiding the real checks, better to be explicit and have them expanded here. |
||
} | ||
|
||
if ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -282,21 +282,8 @@ export const isJSRequest = (url: string): boolean => { | |
return false | ||
} | ||
|
||
const knownTsRE = /\.(?:ts|mts|cts|tsx)$/ | ||
const knownTsOutputRE = /\.(?:js|mjs|cjs|jsx)$/ | ||
const knownTsRE = /\.(?:ts|mts|cts|tsx)(?:$|\?)/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI This import { writeFileSync } from "fs";
import { fileURLToPath } from "url";
import { createServer } from "vite";
writeFileSync(
"./source.ts",
`import { hello } from "./target.mjs"; // Change this to ".mts" or omit extension and it works too
hello();`,
"utf8"
);
writeFileSync(
"./target.mts",
`export const hello = () => "Hello from .mts";`,
"utf8"
);
const __dirname = fileURLToPath(new URL(".", import.meta.url));
const server = await createServer({
configFile: false,
root: __dirname,
server: {
port: 1337,
},
});
await server.listen();
const first = await server.transformRequest("./source.ts");
console.log("First worked!", first.code);
await new Promise((resolve) => setTimeout(resolve, 2000));
const second = await server
.transformRequest("./source.ts?v=123")
.then((result) => console.log("Second worked!", result.code))
.catch((e) => console.error("Second failed", e));
await server.close();
process.exit(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome! Thanks for letting us know! Maybe not a bad idea to send a PR to add a case for this to one of our playgrounds if you would like. |
||
export const isTsRequest = (url: string): boolean => knownTsRE.test(url) | ||
export const isPossibleTsOutput = (url: string): boolean => | ||
knownTsOutputRE.test(cleanUrl(url)) | ||
|
||
const splitFilePathAndQueryRE = /(\.(?:[cm]?js|jsx))(\?.*)?$/ | ||
export function getPotentialTsSrcPaths(filePath: string): string[] { | ||
const [name, type, query = ''] = filePath.split(splitFilePathAndQueryRE) | ||
const paths = [name + type.replace('js', 'ts') + query] | ||
if (type[type.length - 1] !== 'x') { | ||
paths.push(name + type.replace('js', 'tsx') + query) | ||
} | ||
return paths | ||
} | ||
|
||
const importQueryRE = /(\?|&)import=?(?:&|$)/ | ||
const directRequestRE = /(\?|&)direct=?(?:&|$)/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are testing these in the resolve playground