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

ts-transform-paths needs updating for TypeScript 4.5+ #37

Open
Kadeluxe opened this issue Nov 27, 2021 · 13 comments · May be fixed by #38
Open

ts-transform-paths needs updating for TypeScript 4.5+ #37

Kadeluxe opened this issue Nov 27, 2021 · 13 comments · May be fixed by #38

Comments

@Kadeluxe
Copy link

Kadeluxe commented Nov 27, 2021

ts-transform-paths is broken for TypeScript 4.5+.
The reason is that import declarations nodes miss parent node and therefore TS is unable to determine source file of node (compiler/checker.ts, resolveExternalModule function).

I did a quick look and apparently setting newNode.parent to original node .parent fixes it (as you have done for export declarations).
Since I heavily rely on @zerollup/ts-transform-paths in my work, I created @Kadeluxe/ts-transform-paths repo with that fix and some other small changes.

I'm not creating PR here because I don't have much knowledge of TypeScript internals so I can't tell if it's the proper way to fix it. I guess this plugin may need further updating.

UPDATE: since there is no progress on this, I published my fork on NPM: https://www.npmjs.com/package/@kadeluxe/ts-transform-paths
Do npm i @kadeluxe/ts-transform-paths and replace @zerollup/ts-emit-import-alias with @kadeluxe/ts-emit-import-alias in your tsconfig.json. Now it will work.
Also you can switch to different transformer like https://github.com/LeDDGroup/typescript-transform-paths.

@falkenhawk
Copy link

@Kadeluxe could you instead fork this repo and add your fixes and changes in separate commits, please? Otherwise it is impossible to tell what and where your modifications are.

@Kadeluxe
Copy link
Author

Well I didn't really want to bother with the whole monorepo but here you go:
https://github.com/Kadeluxe/zerollup/tree/master/packages/ts-transform-paths
Also published to GitHub Packages.

Again, I didn't really look much into internals, TypeScript has lots of changes since 3.7.2 which this transformers initially targets apparently but this change fixes compilation on TS 4.5 and I haven't noticed other issues yet.

@khitrenovich
Copy link

@zerkalica / @falkenhawk -
Thank you for maintaining ts-transform-paths!
Do you plan to release a patch that fixes TS 4.5 compatibility?

@Drapegnik
Copy link

hi! thanks!
I created patch with https://github.com/ds300/patch-package to temporarily fix it without fork:

just add this to patches/@zerollup+ts-transform-paths+1.7.18.patch file

diff --git a/node_modules/@zerollup/ts-transform-paths/dist/index.js b/node_modules/@zerollup/ts-transform-paths/dist/index.js
index e9da4bd..555f4af 100644
--- a/node_modules/@zerollup/ts-transform-paths/dist/index.js
+++ b/node_modules/@zerollup/ts-transform-paths/dist/index.js
@@ -169,7 +169,9 @@ function importPathVisitor(node, _a) {
         newNode.flags = node.flags;
     }
     if (ts.isImportDeclaration(node)) {
-        newNode = ts.updateImportDeclaration(node, node.decorators, node.modifiers, node.importClause, newSpec);
+        var importNode = ts.updateImportDeclaration(node, node.decorators, node.modifiers, node.importClause, newSpec);
+        importNode.moduleSpecifier.parent = node.moduleSpecifier.parent
+        newNode = importNode
         /**
          * Without this hack ts generates bad import of pure interface in output js,
          * this causes warning "module has no exports" in bundlers.

and following in your package.json

"postinstall": "patch-package"

@embryCODE
Copy link

@Drapegnik You, sir, are a gentleman and a scholar. Thanks for this timely fix.

@krisdages
Copy link

krisdages commented Dec 2, 2021

@Drapegnik Is that "if isImportDeclaration" branch the only one that's actually affected? What if you have an export { ... } from ""?

I should have looked here before trying to figure out the issue myself, but this was the change I made just before returning from that function that checks the node type.

patches/@zerollup+ts-transform-paths+1.7.18.patch file:

diff --git a/node_modules/@zerollup/ts-transform-paths/dist/index.js b/node_modules/@zerollup/ts-transform-paths/dist/index.js
index e9da4bd..10e9832 100644
--- a/node_modules/@zerollup/ts-transform-paths/dist/index.js
+++ b/node_modules/@zerollup/ts-transform-paths/dist/index.js
@@ -217,6 +217,10 @@ function importPathVisitor(node, _a) {
     if (ts.isModuleDeclaration(node)) {
         newNode = ts.updateModuleDeclaration(node, node.decorators, node.modifiers, newSpec, node.body);
     }
+    if (newNode) {
+        newSpec.parent = newNode
+        newNode.parent = node.parent
+    }
     return newNode;
 }
 

@Kadeluxe
Copy link
Author

Kadeluxe commented Dec 4, 2021

TypeScript compiler currently only needs parent for import declarations (starting with 4.5+) but yes, it may be "future-proof" to properly preserve newNode.parent for any cases.
Not sure how it affects the whole thing though, without deep knowledge of TypeScript internals it's hard to tell why TS doesn't preserve parent itself when you call update... compiler methods and what possible issues that may trigger overall.

@alexn-s
Copy link

alexn-s commented Dec 23, 2021

hi! thanks! I created patch with https://github.com/ds300/patch-package to temporarily fix it without fork:

just add this to patches/@zerollup+ts-transform-paths+1.7.18.patch file

diff --git a/node_modules/@zerollup/ts-transform-paths/dist/index.js b/node_modules/@zerollup/ts-transform-paths/dist/index.js
index e9da4bd..555f4af 100644
--- a/node_modules/@zerollup/ts-transform-paths/dist/index.js
+++ b/node_modules/@zerollup/ts-transform-paths/dist/index.js
@@ -169,7 +169,9 @@ function importPathVisitor(node, _a) {
         newNode.flags = node.flags;
     }
     if (ts.isImportDeclaration(node)) {
-        newNode = ts.updateImportDeclaration(node, node.decorators, node.modifiers, node.importClause, newSpec);
+        var importNode = ts.updateImportDeclaration(node, node.decorators, node.modifiers, node.importClause, newSpec);
+        importNode.moduleSpecifier.parent = node.moduleSpecifier.parent
+        newNode = importNode
         /**
          * Without this hack ts generates bad import of pure interface in output js,
          * this causes warning "module has no exports" in bundlers.

and following in your package.json

"postinstall": "patch-package"

Awesome fix. worked perfectly.

thank you so much

@pmrotule
Copy link

pmrotule commented Jan 4, 2022

Well I didn't really want to bother with the whole monorepo but here you go: https://github.com/Kadeluxe/zerollup/tree/master/packages/ts-transform-paths Also published to GitHub Packages.

@Kadeluxe Why not opening a PR? That would be very helpful.

simmo added a commit to simmo/zerollup that referenced this issue Jan 10, 2022
@simmo simmo linked a pull request Jan 10, 2022 that will close this issue
@simmo
Copy link

simmo commented Jan 10, 2022

@Drapegnik Is that "if isImportDeclaration" branch the only one that's actually affected? What if you have an export { ... } from ""?

I should have looked here before trying to figure out the issue myself, but this was the change I made just before returning from that function that checks the node type.

patches/@zerollup+ts-transform-paths+1.7.18.patch file:

diff --git a/node_modules/@zerollup/ts-transform-paths/dist/index.js b/node_modules/@zerollup/ts-transform-paths/dist/index.js
index e9da4bd..10e9832 100644
--- a/node_modules/@zerollup/ts-transform-paths/dist/index.js
+++ b/node_modules/@zerollup/ts-transform-paths/dist/index.js
@@ -217,6 +217,10 @@ function importPathVisitor(node, _a) {
     if (ts.isModuleDeclaration(node)) {
         newNode = ts.updateModuleDeclaration(node, node.decorators, node.modifiers, newSpec, node.body);
     }
+    if (newNode) {
+        newSpec.parent = newNode
+        newNode.parent = node.parent
+    }
     return newNode;
 }
 

I've raised a PR for this patch - #38

@perzeuss
Copy link

perzeuss commented Mar 31, 2022

You can use another transformer, e.g. { "transform": "typescript-transform-paths" } which can be used as a replacement for @zerollup/ts-transform-paths if you don't need specific functionality that only this library has.

We go with another transformer instead of using a patch.

@bdwain
Copy link

bdwain commented Sep 22, 2022

I tried this patch but it still seems to fail for me. Anyone else seeing that with the latest versions of things?

@bdwain
Copy link

bdwain commented Sep 22, 2022

i think I have may have found a more sustainable solution to this problem, for anyone still looking.

Microsoft has a tool called api extractor. Basically it'll parse your d.ts files and then do a few different things with them, one of which is to make a d.ts rollup by combining your d.ts files into a single file.

One of the configuration options this tool accepts is a tsconfig file. I took my project's tsconfig and modified it so that the aliases were pointing at the generated types, rather than the src code. For example, change

      paths: {
        'src/*': ['src/*'],
      },

to

      paths: {
        'src/*': ['es/types/src/*'],
      },

Then when the tool runs, it generates a single d.ts file that has no aliases in it anymore because it optimized away all imports except imports of third party types.

This seems to work well, and makes ts-transform-paths unnecessary I think.

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 a pull request may close this issue.