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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(typescript) replace getOutputFileNames #726

Closed
wants to merge 1 commit into from
Closed

fix(typescript) replace getOutputFileNames #726

wants to merge 1 commit into from

Conversation

Samantha-uk
Copy link

@Samantha-uk Samantha-uk commented Dec 10, 2020

Rollup Plugin Name: {@rollup/plugin-typescript}

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no (I don't believe so)

List any relevant issue numbers:
An issue I experienced (details below) and rather than logging it, I set out to fix it.
Potentially resolves #287
Potentially resolves #608

Description

The problem I experienced was that when I used the Rollup API, if I set tsconfig:false and passed in all of my compilerOptions to the typescript plugin, the system would fail ... with a very unhelpful message.

Turns out is was a failing assertion deep inside typescript as part of the getOutputFileNames call chain. (Little did I realise at the time that it had been causing grief for others in a different fashion).

So I set out to see if there was another approach...

It turns out that TS knows (obviously) and _already tells the typescript plugin where it has generated files and what the corresponding source files are, this comes via the BuildProgram writeFile callback. By utilising the extra information this PR obviates the need for getOutputFileNames an external call we no longer need to make. 馃コ

I've added a new test (that failed pre these changes) and I've slightly tweaked a couple of others as the order of files reported to the test harness has changed and the FakeTypescript test needed to also pass in the newly used source file information.

I also changed the way that the generateBundle figures out the baseDir path to use ... basing it on the approach MS document on their TS site (URL in the source) ... however, whilst it works and does not break tests ... I feel that there is perhaps a better way to calculate this ... perhaps just copying the hierarchy that the TS program generates? 馃

There is a possibility that this may also address issue #287 however I was unsuccessful in getting the test that @benmccann added ... but I think it is close ... 馃

@Samantha-uk
Copy link
Author

Oh dear ... how embarrassing that the tests are failing.
I'm running Debian buster / node 14.15.1 and they all pass.
image
Any ideas of what I may need to change?

@benmccann
Copy link
Contributor

Not sure about the failing tests, but I wonder if the issue you were facing would have been fixed by microsoft/TypeScript#41811. This seems like potentially extra code in the plugin that might not really be necessary after upgrading TypeScript

@Samantha-uk
Copy link
Author

I saw that and It may be fixed in that but not soon. The PR changes are working nicely in my build environment so I鈥檓 thankfully unblocked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants