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

Use PluginContext.emitFile to emit declarations (when possible) #181

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

marijnh
Copy link
Contributor

@marijnh marijnh commented Oct 11, 2019

See issue #180

See this comment for an explanation of the logic that chooses between writeFile and emitFile.

@ezolenko
Copy link
Owner

Looks good, but it is better if declarations are either all written or all emitted. So if pluginOptions.useTsconfigDeclarationDir is true, use tsModule.sys.writeFile for everything, and if not, emit everything. It would be easy to document in readme then and there will be no surprises downstream. Things should either work or not work, worst failure mode is when some files suddenly don't get processed by downstream plugins just because folder structure changed.

To resolve naming conflicts in the second case, path separators can be replaced with underscores, so outdir/subdir/name.d.ts becomes outdir/subdir_name.d.ts.

@ezolenko ezolenko self-requested a review October 12, 2019 04:23
Copy link
Owner

@ezolenko ezolenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment

@marijnh
Copy link
Contributor Author

marijnh commented Oct 12, 2019

Always directly writing with useTsconfigDeclarationDir is a good idea. Implemented in the updated patch.

I don't quite get your suggestion to substitute underscores. We can emit subdirectories (they just have to have the shape foo/bar.d.ts, not ../bar.d.ts), and mangling file names would break imports of those files from other declaration files.

@ezolenko ezolenko merged commit 551a14e into ezolenko:master Oct 15, 2019
@ezolenko
Copy link
Owner

Looks good, thanks. I'll make a release sometime this week hopefully.

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