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

Why does the plugin write declaration files directly? vs using emitFile #180

Closed
marijnh opened this issue Oct 9, 2019 · 8 comments
Closed
Labels
kind: feature New feature or request scope: upstream Issue in upstream dependency

Comments

@marijnh
Copy link
Contributor

marijnh commented Oct 9, 2019

As opposed to using emitFile on the rollup context?

The main issue I'm running into is that declarations aren't emitted at all when using the rollup API directly, because that calls the generateBundle hook with isWrite==false. But more generally, it would be useful to have these in the return value from generate, so that I can feed them into a .d.ts bundling tool.

@marijnh
Copy link
Contributor Author

marijnh commented Oct 9, 2019

As an additional, possibly obvious data point, if I just crudely replace the tsModule.sys.writeFile call with

cx.emitFile({
  type: "asset",
  source: entry.text,
  fileName: require("path").basename(fileName)
})

where cx is the this from generateBundle, the plugin seems to continue to correctly emit declaration files. Though I guess just passing the basename as fileName isn't correct when the declaration files have their own directory structure.

@ezolenko ezolenko added kind: feature New feature or request help wanted labels Oct 10, 2019
@ezolenko
Copy link
Owner

I think this API didn't exist when the plugin was originally written (or I missed it completely). This does make a lot of sense though, feel free to make a PR. (If not, I'll fix it, eventually :)).

@marijnh
Copy link
Contributor Author

marijnh commented Oct 11, 2019

Working on a PR, but for some random reason the filenames provided for emitted assets can not be relative or absolute paths (can only emit inside of Rollup's output dir), which makes it hard to respect the useTsconfigDeclarationDir option (which may be anywhere). Do you have an idea on how to address this? Is directly writing the file ourselves when the file name is outside of the output dir too much of a kludge?

marijnh added a commit to marijnh/rollup-plugin-typescript2 that referenced this issue Oct 11, 2019
marijnh added a commit to marijnh/rollup-plugin-typescript2 that referenced this issue Oct 12, 2019
@ezolenko ezolenko added the kind: bug Something isn't working properly label Oct 15, 2019
ezolenko added a commit that referenced this issue Oct 15, 2019
@ezolenko
Copy link
Owner

Looks like there is a problem with emitting files during generateBundle -- multiple targets will cause same file being emitted multiple times and conflict.

If I track what was emitted and not emit it after first target, watch mode breaks (doesn't update types in all targets).

Try running build, then build-self, then build-self again to see it.

I fixed part of it in emit branch, but types-only files are still a problem, because those are known only at generate stage.

@marijnh
Copy link
Contributor Author

marijnh commented Oct 16, 2019

Strange. My own build process also uses multiple targets for a single bundle, and doesn't run into this error. It uses bundle.generate instead of bundle.write, but those seems to call the generateBundle hook in the same way.

It does seem like the bundle object is reused between calls to generate or write, which I don't really get (it would seem each write generates its own files).

But even when I pass the bundle object (second parameter to generateBundle) to the emit code and disable emitting when the file is already present, the error persists. But I must say I'm quite confused by the plugin's build system—firstly, there's weird line locations in rollup.config.self.js in the stack trace for the build failure (see below), secondly, it doesn't actually seem to be running node_modules/rollup/dist/rollup.js, despite that appearing in the stack trace—if I edit that to add additional logging the logging doesn't show up. And despite disabling the emitFile call, the error still appears. Can you see what I'm doing wrong? (This is what I've been trying, which, further correctness questions aside, seems like it should at least avoid this error.)

[!] (plugin rpt2) Error: Could not emit file "index.d.ts" as it conflicts with an already emitted file.
Error: Could not emit file "index.d.ts" as it conflicts with an already emitted file.
    at error (/home/marijn/tmp/rollup-plugin-typescript2/node_modules/rollup/dist/rollup.js:10162:30)
    at reserveFileNameInBundle (/home/marijn/tmp/rollup-plugin-typescript2/node_modules/rollup/dist/rollup.js:16228:16)
    at FileEmitter.emitAsset (/home/marijn/tmp/rollup-plugin-typescript2/node_modules/rollup/dist/rollup.js:16352:17)
    at Object.FileEmitter.emitFile (/home/marijn/tmp/rollup-plugin-typescript2/node_modules/rollup/dist/rollup.js:16282:29)
    at emitDeclaration (/home/marijn/tmp/rollup-plugin-typescript2/rollup.config.self.js:27470:26)
    at /home/marijn/tmp/rollup-plugin-typescript2/rollup.config.self.js:27478:17
    at /home/marijn/tmp/rollup-plugin-typescript2/rollup.config.self.js:4929:15
    at baseForOwn (/home/marijn/tmp/rollup-plugin-typescript2/rollup.config.self.js:3014:24)
    at /home/marijn/tmp/rollup-plugin-typescript2/rollup.config.self.js:4898:18
    at forEach (/home/marijn/tmp/rollup-plugin-typescript2/rollup.config.self.js:9366:14)

@ezolenko
Copy link
Owner

ezolenko commented Oct 16, 2019

Confusing stack trace is probably because of the way rollup consumes rollup.config.js -- those line numbers are completely bogus, you have to search by function names.

I just tried adding a console.log call in reserveFileNameInBundle in rollup-plugin-typescript2/node_modules/rollup/dist/rollup.js and I see it. The line number is off by 800 or so though.

When I tried limiting emits here it kinda worked, but I see weird results in watch.

When testing plugin's own build on your changes, first do npm build to build plugin with a released build of the plugin. This will populate build-self target. Then do npm build-self, this will use just built local build. Then you can do another round of npm build-self, to make sure plugin built by itself can still build itself. :)

If you get to the point where your changes can't do a successful build. reset with npm build.

@marijnh
Copy link
Contributor Author

marijnh commented Oct 18, 2019

Okay, it seems that the bundle object passed to generateBundle isn't actually the bundle that is used by rollup to check for duplicate names. I've spent some time trying to figure out how plugins are supposed to do this, but neither the rollup docs nor the code are really helping.

I've tried to get some feedback on this from the rollup devs but I'm not sure if that'll go anywhere. For now, maybe it makes sense to revert the original patch.

@marijnh
Copy link
Contributor Author

marijnh commented Oct 18, 2019

This is apparently not intentional behavior in Rollup, see rollup/rollup#3174

@agilgur5 agilgur5 changed the title Why does the plugin write declaration files directly? Why does the plugin write declaration files directly? vs using emitFile Jul 14, 2022
@agilgur5 agilgur5 added scope: upstream Issue in upstream dependency and removed kind: bug Something isn't working properly help wanted labels Jul 14, 2022
Repository owner locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind: feature New feature or request scope: upstream Issue in upstream dependency
Projects
None yet
Development

No branches or pull requests

3 participants