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

Rollup plugin URL should use the emitFile API #489

Closed
bastienrobert opened this issue Jul 7, 2020 · 7 comments
Closed

Rollup plugin URL should use the emitFile API #489

bastienrobert opened this issue Jul 7, 2020 · 7 comments

Comments

@bastienrobert
Copy link

  • Rollup Plugin Name: @rollup/plugin-url
  • Rollup Plugin Version: *

Expected Behavior / Situation

It should use the emitFile API to prevent watch to infinitely loop.

Actual Behavior / Situation

Actually, the plugin is using FS and is writing to the disk which causes the file watcher to trigger a rebuild.

I think it'll be great to update the doc to specify it's a bad practice (https://rollupjs.org/guide/en/#thisemitfileemittedfile-emittedchunk--emittedasset--string).

Modification Proposal

Ideally the plugin should be updated to the use the proper emitFile API, or change the plugin hook to writeBundle instead which makes more sense anyways than generateBundle.
PepsRyuu

@shellscape
Copy link
Collaborator

We'd be happy to review a PR to resolve this.

@bastienrobert
Copy link
Author

It’s under investigation 😉

@stale stale bot added the x⁷ ⋅ stale label Sep 7, 2020
@stale
Copy link

stale bot commented Sep 8, 2020

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@stale stale bot closed this as completed Sep 8, 2020
@azerella
Copy link

azerella commented Sep 13, 2020

@bastienrobert can we reopen this? I also believe this is an issue

@shellscape
Copy link
Collaborator

@adamzerella The bot's message looks clear to me - this requires community resolution, and none was provided, so the issue was stale and closed. The same will happen to the new issue you opened if no one steps up to help resolve it.

@jacksteamdev
Copy link
Contributor

jacksteamdev commented Dec 22, 2020

I can see why this issue went stale. I was considering a PR, but this plugin does much more than just emit assets.

This plugin has a lot of customization options that make a conversion more complex: fileName, publicPath, sourceDir, and destDir. Some of these options might be better handled by another plugin or option: sourceDir is probably an alias. fileName and destDir are things that might be handled by assetFileNames, but would this be desirable? These would be breaking changes, and these are probably features people use.

It seems that the design of this plugin allows for a dest folder outside of the Rollup dest dir. It can be configured to break watch mode by outputting to a folder that Rollup is watching. Is this something we want to allow?

This plugin uses a node stream to copy the files. It's the most memory efficient way to do it, and to use the emitFile API would require the files to be loaded into memory. Probably not desirable.

All in all, I think a new plugin without the extra options would be better for the most common use cases. Maybe @rollup/plugin-url-assets?

@art-solopov
Copy link

I second the issue. I'm guessing not using the emitFile API is why the manifest plugin can't handle the generated files (normally it handles all assets).

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

No branches or pull requests

5 participants