-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Add support for rewriting imports in declaration files #1
Conversation
This PR appears to be targeting main on your fork, rather than upstream main. Was this intentional? |
src/worker/worker.ts
Outdated
@@ -62,6 +63,9 @@ export class Worker { | |||
private getJSMapPath(path: string): string { | |||
if (!this.data.extname) return path; | |||
|
|||
// const ext = this.data.extname === ".mjs" ? "" : ""; |
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.
Not needed?
function isRelativePath(path: string): boolean { | ||
return path.startsWith("./") || path.startsWith("../"); | ||
} | ||
export function createRewriteDtsImportTransformer( |
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.
Docs explaining all of what this is intended to do would be helpful for reviewers
Yes. It's not anywhere near ready to open as an upstream change. There are no tests, plus it's lacking features that would be expected in a "real implementation." I'd love to do that extra work, but I don't have the time now. |
This PR patches the repo version of tsc-multi with the contents of this PR: tylerbutler/tsc-multi#1 This change will enable tsc-multi to rewrite the local imports in declaration (.d.ts) files just like it does for js files. This is opt-in behavior in the tsc-multi config. tsc-multi will not rename the declaration files so that will need to be done separately (see the changes in #18645 for examples of how we're going to do the renames).
src/worker/worker.ts
Outdated
* Rewrites an output file path based on its extension. If ignoreDts is true, then paths to .d.ts files will not be | ||
* rewritten. This is used so that .d.ts file paths are only rewritten under specific circumstances. | ||
*/ | ||
private rewritePath(path: string, ignoreDts: boolean): string { |
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.
I am not a huge fan of defaults, but I like flat boolean parameters less. (You can't see the argument at the call site.)
So, I would make ignoreDts optional and not pass false
at all of the regular uses.
There is a comment at the special case, but I admit I don't know why it is special after reading the comment. Lingering thoughts of why?
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.
Updated to have a default value false
. I don't understand why it's a special case, but I can at least add details about what happens when we rewrite in the read function. I'll do another test and add those details.
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.
One drawback to default false is that it changes the logic even when not "opted in" to dts rewriting. That said, if they're not opted in, then all the dts extensions should be unchanged, so even though the codepaths are different the end result should be the same.
src/worker/worker.ts
Outdated
private getDtsPath(path: string): string { | ||
if (!this.data.dtsExtName) return path; | ||
|
||
return trimSuffix(path, DTS_EXT) + this.getDtsExtension(); |
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.
.data.dtsExtName
doesn't work here like it .data.extname
works for getJSPath
?
It should - also makes the test for !this.data.dtsExtName
just above meaningful.
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.
Good point. the getDtsExtension is no longer needed. I originally had some central logging there but removed it after debugging.
src/worker/worker.ts
Outdated
return JSON.stringify(json); | ||
} | ||
|
||
private createSystem(sys: Readonly<ts.System>): ts.System { | ||
const getReadPaths = (path: string) => { | ||
const paths = [this.rewritePath(path)]; | ||
// When reading paths, we don't want to rewrite the paths to DTS files, so we pass true to ignoreDts |
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.
Why don't we want to rewrite them like we would for .js? Can you add a little more commentary? Thanks!
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.
I added some more comments. If we rewrite the paths, there are errors like this:
error during command 'tsc-multi --config ../../../common/build/build-common/tsc-multi.esm.json' (exit code 1)
[mjs]: error TS6053: File '/home/tylerbu/code/release-1/node_modules/.pnpm/typescript@5.1.6/node_modules/typescript/lib/lib.dom.d.ts' not found.
The file is in the program because:
Library 'lib.dom.d.ts' specified in compilerOptions
[mjs]: error TS6053: File '/home/tylerbu/code/release-1/node_modules/.pnpm/typescript@5.1.6/node_modules/typescript/lib/lib.dom.iterable.d.ts' not found.
The file is in the program because:
Library 'lib.dom.iterable.d.ts' specified in compilerOptions
[mjs]: error TS6053: File '/home/tylerbu/code/release-1/node_modules/.pnpm/typescript@5.1.6/node_modules/typescript/lib/lib.es2020.d.ts' not found.
The file is in the program because:
Library 'lib.es2020.d.ts' specified in compilerOptions
[mjs]: error TS2318: Cannot find global type 'Array'.
[mjs]: error TS2318: Cannot find global type 'Boolean'.
[mjs]: error TS2318: Cannot find global type 'CallableFunction'.
[mjs]: error TS2318: Cannot find global type 'Function'.
[mjs]: error TS2318: Cannot find global type 'IArguments'.
[mjs]: error TS2318: Cannot find global type 'NewableFunction'.
[mjs]: error TS2318: Cannot find global type 'Number'.
[mjs]: error TS2318: Cannot find global type 'Object'.
[mjs]: error TS2318: Cannot find global type 'RegExp'.
[mjs]: error TS2318: Cannot find global type 'String'.
[mjs]: Found 13 errors.
I think that what's happening is that the build process uses the provided read method to read built-in types, so when we rewrite their paths, TypeScript later can't find them looking at the rewritten paths. That's just a guess though.
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.
I did not review the big cloned file. I think I'd get lost without seeing diff.
Otherwise, I have a few suggestions but didn't find anything problematic.
Do be sure to update the Feature expected but not included portion of description.
src/worker/entry.ts
Outdated
const data = await loadWorkerData(); | ||
data.dtsExtName = data.target.dtsExtName as string; | ||
debug("Target", data.target); |
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.
See #1 (comment)
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.
LGTM (split file caveat for review remains)
This PR adds minimal support for rewriting imports in declaration (.d.ts) files by adding an "afterDeclarations" handler to supplement the existing "after" handler. To enable this feature, set the
dtsExtName
config value in the tsc-multi target config. The value should be the file extension of the declaration files you want. Sourcemap files and their references are also rewritten. Their extensions will match the configureddtsExtName
with.map
appended.This is an example configuration that configures an ESNext output using the
.mjs
extension for code files and.d.mts
extension for declaration files.If
dtsExtName
is omitted, behavior should match exactly what it is today without this feature.To keep the behavior separate, I copied the existing transformer and adjusted it specifically for declarations. This way any issues with the declaration files is clearly because of the new code - it shares no functional code with the old transformer.
There is some shared code in worker.ts to rename file extensions, but even in that file most logic is clearly separated so without opting in via config, behavior should match what it is today.