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 4.3 support #544

Closed
wants to merge 5 commits into from

Conversation

BrianLeishman
Copy link

So I got tscc to work with TS 4.3 now that tsickle supports it, but there's gray areas that I'm not 100% about.

First thing, it seems like tsickle has an issue installing on version 0.40.0 from npm where the package.json points to the wrong dir for its own files. I made an issue with them about that angular/tsickle#1272. That's what the patch file in this commit does.

Second is with the const sf = <SourceFileWithInternalAPIs>this.host.getSourceFile(fileName); line. This seems to be related this issue #464. In my own environment, I've jst been skipping the next actions if that line returns something falsey. I've not personally experienced any cons from that, but it feels like maybe not the right solution,

Lastly, with the const transformerHost: tsickle.TsickleHost = { line, it seems like this version of tsickle requires a new function as part of the tsickle.TsickleHost interface, called rootDirsRelative that takes a filename and returns a string. I couldn't find anything about what specifically this does, so I just wrote it to return what was passed to it and my compiled code seems to be working correctly, but again, not sure that's the right solution.

@BrianLeishman
Copy link
Author

BrianLeishman commented Jun 15, 2021

tsickle fixed the issue with the package.json file so it's all good to go now with tsickle version 0.43.0

@theseanl
Copy link
Owner

Much appreciated! I will try to wrap my head around the recent changes of tsickle and review it. It's disappointing that they haven't improved in terms of module-.d.ts support.

I guess CompilerHost.getSourceFile returning nullish requires a bit of consideration. Regarding rootDirsRelative, I guess you are right, because as I remember, back then there were a lot of path-related issues with tsickle, and I just forced tscc to use absolute paths everywhere whenever possible.

@theseanl
Copy link
Owner

theseanl commented Jul 12, 2021

Hi, the change seemed mostly fine, I made a few quick changes and squash-merged with the master (I didn't know that your account is not displayed in commit log if I do this).

  • Regarding the new requirement of rootDir compiler option
    • It is pointless in tscc, as tscc has been working just fine without rootDir option. Instead of commenting out some lines as you have did, I went to modify rootFile property to a some dummy path. It is set to a root path of a current config file's path. In case of linux, it will always be /, and in Windows, it will be something like C:\\ .
  • Regarding !sf check: I can't make a reproduction case of what you have described, so removed this patch. Please file a new issue with steps to reproduce the issue.
  • This repository uses Yarn, so I have deleted package-lock.json and updated yarn.lock.
  • I also clarified some comments.

@theseanl theseanl closed this Jul 12, 2021
@BrianLeishman
Copy link
Author

Yeah I figured the yarn/npm thing would come up. I learned about yarn vs npm over the last week or so, but before that I was struggling with the yarn side, and only npm was working for me, but of course it put it's package-lock file there.

I'll see if I can get a reproducible build for the issue about those dependencies that seem to be missing causing issues with the !sf line/check

@theseanl
Copy link
Owner

Well, I just wanted to make sure I mention every change and its reason, what package manager to use is a very marginal issue I guess. While waiting for tsickle release, my attention was slowly slipping over to other tasks. Thank you for bring this up and providing & testing necessary changes! Also now tscc 0.7.0 with typescript 4.3.2 is live in NPM.

@BrianLeishman
Copy link
Author

That's awesome, thank you!

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 this pull request may close these issues.

2 participants