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 shouldn't close stdout or subsequent writes will fail. #4995

Closed
mlstich opened this issue May 16, 2023 · 6 comments · Fixed by #5004
Closed

Rollup shouldn't close stdout or subsequent writes will fail. #4995

mlstich opened this issue May 16, 2023 · 6 comments · Fixed by #5004

Comments

@mlstich
Copy link

mlstich commented May 16, 2023

Expected Behavior / Situation

The expected behaviour is the one before #4983 - not to close stdout with process.stdout.end(). Subsequent writes to stdout (through node) will continue to be successful.

Actual Behavior / Situation

With #4983 / Rollup v3.21.6, stdout is closed. Subsequent writes to stdout will fail.

Our specific case:

Modification Proposal

Roll back the change in #4983 (and solve it differently, without affecting the node process beyond Rollup).

@lukastaegert
Copy link
Member

Are you calling Rollup CLI as a Node script instead of as a separate process? How is it a problem that we close stdout but not that we call process.exit? the change was introduced because explicitly calling process.exit was aborting the CLI before stdout was fully written. Explicit process.exit on the other hand solved issues with Rollup not closing in certain configurations.

mlstich added a commit to mlstich/rollup-stdout-repro that referenced this issue May 16, 2023
mlstich added a commit to mlstich/rollup-stdout-repro that referenced this issue May 16, 2023
mlstich added a commit to mlstich/rollup-stdout-repro that referenced this issue May 16, 2023
@mlstich
Copy link
Author

mlstich commented May 16, 2023

Are you calling Rollup CLI as a Node script instead of as a separate process?

Yes, you can put it that way. The GitHub action does that. Using the firebase CLI alone doesn't produce the issue.

How is it a problem that we close stdout but not that we call process.exit

Good point! I'm not familiar with how processes are managed in node unfortunately.

I created a repro to demonstrate the issue: https://github.com/mlstich/rollup-stdout-repro.

@mlstich
Copy link
Author

mlstich commented May 19, 2023

Workaround (as an alternative to pinning the dependency): Redirect stdout to /dev/null when invoking rollup:

rollup main.js --file bundle.js > /dev/null

@lukastaegert
Copy link
Member

Fix at #5004

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #5004 as part of rollup@3.22.1. You can test it via npm install rollup.

@mlstich
Copy link
Author

mlstich commented May 22, 2023

Awesome, thanks for the fix!

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

Successfully merging a pull request may close this issue.

3 participants