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

typescript v4 support #68

Closed
vladimiry opened this issue Aug 21, 2020 · 9 comments · Fixed by #69
Closed

typescript v4 support #68

vladimiry opened this issue Aug 21, 2020 · 9 comments · Fixed by #69
Labels
Bug Something isn't working

Comments

@vladimiry
Copy link

vladimiry commented Aug 21, 2020

[email protected]
[email protected]

typescript-transform-paths/lib/index.js:106
            return updaterFn(typescript_1.default.createLiteral(p));
                   ^
TypeError: typescript_1.default.updateNode is not a function
@danielpza danielpza added the Bug Something isn't working label Aug 21, 2020
@nonara
Copy link
Collaborator

nonara commented Aug 21, 2020

Thanks for the report, @vladimiry

@danielpza - This is due to the fact that updateNode was an internal function. TS 4 did a total overhaul of the node factory API. Cool, but it's going to be a headache for me in a lot of areas.

If you get a chance to have a look at this (before I can), I'd like to see what happens if we drop the workaround and try it using the normal methods. (updateImportDeclaration, etc) It's possible that we don't need the workaround anymore, since they gutted and rebuilt the internal methods. We're looking to see if it still erroneously adds type-only exports in compiled source after we do an update on import or export declaration nodes.

This bug report was filed, but the OP closed it due to finding the workaround - which now no longer works. If we're lucky, this won't be a problem with the new factory system. If it is still a problem, I'll file a new bug report with TS and probably just bite the bullet and do a PR for TS that fixes it so we can publish a fix for TS 4.

@vwpo
Copy link

vwpo commented Sep 15, 2020

[email protected]
[email protected]

Confirm that plugin doesn't replace paths in declaration files but it works with [email protected]

@nonara
Copy link
Collaborator

nonara commented Sep 16, 2020

Sorry that this took awhile to get to. I've been completely swamped.

The good news is, the upgrade for TS4 compatibility is done and was painless.
The bad news is, the bug in the compiler is still not fixed. 😞

This means that type specifiers in and import or export declaration get output in the JS, which is not good.

I'm investigating it now to see if I can determine what's happening. Stay tuned...

nonara added a commit that referenced this issue Sep 17, 2020
@nonara
Copy link
Collaborator

nonara commented Sep 17, 2020

Fixed. New version incoming: #69

Background

For those interested, I didn't track out exactly what was happening in the compiler. I found a few flags discrepancies on the AST nodes (between transformed and original), however, accounting for them didn't make a difference. There may be an internal map somewhere linking some information to the pre-transformed node that is getting lost after transformation.

The MS team would have more knowledge on that area of the codebase than I do, and I don't have the time to dig further into it; however, I was able to find another method to update the node without causing any issues.

In addition to the fix, I've updated the transformer code to prefer the new NodeFactory property of TransformationContext, when it is available. This will keeps us in compliance with TS4+ as the old methods become deprecated over the next few minor TS releases.

TS Issue

I've also opened a new issue for the compiler, which can be tracked here: microsoft/TypeScript#40603

nonara added a commit that referenced this issue Sep 17, 2020
Closes #68

* Added prettier to devDependencies
* Formatting + added prettier format script
* Update contributors field
@vladimiry
Copy link
Author

Can confirm issue resolving on my case. Thanks everyone involved.

@danielpza
Copy link
Member

danielpza commented Sep 18, 2020

Big thanks to @nonara for putting up the PR 🎉.

@all-contributors add @vladimiry @vwpo for bug

@allcontributors
Copy link
Contributor

@danielpza

I've put up a pull request to add @vladimiry! 🎉

@danielpza
Copy link
Member

@all-contributors also add @vwpo for bug smh

@allcontributors
Copy link
Contributor

@danielpza

I've put up a pull request to add @vwpo! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants