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

Export getOutputJavaScriptFileName from tsbuild #26421

Closed
wants to merge 1 commit into from

Conversation

andrewbranch
Copy link
Member

Fixes #26410

Waiting to hear:

  • Since the function will become public, should there be a unit test added for it, or does coverage by existing tsbuild tests suffice?
  • Should we also export getOutputDeclarationFileName for symmetry? What about getOutputFileNames?
  • Any other feedback about implementation, e.g. is tsbuild.ts still the right place for this now that it's being exported, should we add JSDoc comments to it (some exported functions have it, others don't)?

@andrewbranch
Copy link
Member Author

Interesting that when I first opened this PR, it generated baselines in tests/baselines/reference/api/*.d.ts, and after rebasing against master, the change no longer affects those baselines. Just want to make sure that's intended.

@sheetalkamat
Copy link
Member

tsbuild API is internal right now and we would want to make sure to expose it right way after reworking on it.
cc: @RyanCavanaugh @DanielRosenwasser

@weswigham
Copy link
Member

It no longer affects the API baselines because the entire tsbuild namespace got marked @internal because we didn't intend to expose it, I believe.

@andrewbranch
Copy link
Member Author

👍🏽 Makes sense. In regard to the conversation in TypeStrong/ts-loader#815
TypeStrong/ts-loader#815 (comment):

[Me]: would the team consider exporting this so we don’t have to duplicate the logic?
[@RyanCavanaugh]: certainly.

Does that still stand with all the tsbuild stuff being marked internal? Other tsbuild functions are still exported, right? (Some of the others may be needed for the proposed second phase of ts-loader support, building upstream projects.)

On the other hand, if all these APIs are totally internal and subject to change in patch releases, that makes them a bit hard to rely on in a package like ts-loader. @johnnyreilly thoughts on this?

@johnnyreilly
Copy link

I'd much rather rely on TypeScript APIs than duplicate logic in ts-loader if possible. That said, we'll try and be pragmatic; if it's a simple function that's unlikely to change then it's not the end of the world if we duplicate. But that's very much second preference.

@andrewbranch
Copy link
Member Author

We need to make a decision for ts-loader (TypeStrong/ts-loader#815) in the next couple days, so any update here from the team would be greatly appreciated. Thanks!

@sheetalkamat
Copy link
Member

We are still working on api to use project references and anything from tsbuild wont be available as api till that work is complete.
cc:: @RyanCavanaugh

@andrewbranch
Copy link
Member Author

Ok, thanks for the update. Feel free to close this if it doesn't make sense.

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.

None yet

4 participants