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

Implement stdin input #3312

Merged
merged 9 commits into from Jan 4, 2020
Merged

Implement stdin input #3312

merged 9 commits into from Jan 4, 2020

Conversation

lukastaegert
Copy link
Member

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Closes #1440
Closes #3276
Closes #3290

Description

This continues #3290 by @kzc. Unfortunately, the original repository was removed, which is why I moved it to the main Rollup repo.

Changes:

  • It is only possible to use stdin when using the CLI. The CLI does it by injecting a plugin.
  • When using no config file and providing no input, the CLI will take input form stdin in non-TTY situations (e.g. in a pipe)
  • You can force reading the input from stdin in all environments or when using a config file by adding a new --stdin flag. This will replace all inputs in a config file.
  • When using the CLI by default, using - as module name will read this module from stdin.
  • You can prevent this and make - a regular module name by passing the --no-stdin CLI flag

Documentation is still missing, otherwise it should be fully functional.

@lukastaegert
Copy link
Member Author

Also TODO: Fix tests on Windows.

@codecov
Copy link

codecov bot commented Jan 1, 2020

Codecov Report

Merging #3312 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3312      +/-   ##
==========================================
+ Coverage   93.13%   93.17%   +0.04%     
==========================================
  Files         172      172              
  Lines        6062     6041      -21     
  Branches     1809     1801       -8     
==========================================
- Hits         5646     5629      -17     
  Misses        221      221              
+ Partials      195      191       -4
Impacted Files Coverage Δ
cli/run/index.ts 87.03% <100%> (-0.47%) ⬇️
src/utils/defaultPlugin.ts 94% <100%> (-0.12%) ⬇️
src/utils/mergeOptions.ts 92.04% <100%> (+0.09%) ⬆️
cli/run/stdin.ts 100% <100%> (ø)
src/rollup/index.ts 96.96% <100%> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6106af1...1f3bbda. Read the comment docs.

@lukastaegert
Copy link
Member Author

Ready to be merged now

@kzc
Copy link
Contributor

kzc commented Jan 1, 2020

LGTM. All my command line stdin scenarios continue to work.

Overriding all file inputs in the rollup config with --stdin is interesting. It could be useful for debugging a config. Glad to see that one can still mix stdin "-" inputs with regular named input filenames in a config if --stdin is not specified.

How might one enable stdin functionality from the API if they wanted to? Is the stdin plugin exported or accessible somehow?

Users will likely want to know how to process typescript or other input formats from stdin. Probably not possible unless one would have the ability to specify an implied stdin filename suffix somehow.

@lukastaegert
Copy link
Member Author

How might one enable stdin functionality from the API if they wanted to? Is the stdin plugin exported or accessible somehow?

I was wondering about this but I could not come up with a reasonable scenario where the API user should not rather collect and handle stdin themselves. stdin is a singleton and should IMO be the responsibility of the parent application and not the framework, which is why I moved it to the CLI part.

Users will likely want to know how to process typescript or other input formats from stdin. Probably not possible unless one would have the ability to specify an implied stdin filename suffix somehow.

Interesting question. Maybe we should extend it to allow any suffix with '-'

@kzc
Copy link
Contributor

kzc commented Jan 1, 2020

stdin is a singleton and should IMO be the responsibility of the parent application and not the framework, which is why I moved it to the CLI part.

I suspect that API users requiring stdin input would end up copying cli/run/stdin.ts into their projects if it's not exported. You might consider having API calls to get and override the default stdin plugin.

Maybe we should extend it to allow any suffix with '-'

As long as the default CLI stdin user experience remains the same and assumes ECMAScript, sure.

But neither of these issues is a showstopper and could be implemented in future PRs.

@lukastaegert
Copy link
Member Author

But neither of these issues is a showstopper and could be implemented in future PRs

Agreed, this could all be added in non-breaking PRs.

Overriding all file inputs in the rollup config with --stdin is interesting

It is, but the longer I think about it, it also makes the --stdin flag much more confusing as it is a three-state flag now. It also makes it harder do document. I think I want to change that again so that it does not override stdin but just adds or removes the stdin plugin.

@kzc
Copy link
Contributor

kzc commented Jan 2, 2020

Good call. The --stdin file input override was indeed confusing.

@kzc
Copy link
Contributor

kzc commented Jan 2, 2020

Should you ever decide to expose the stdin plugin used in this PR, be aware that it has a data race if there are multiple callers of readStdin() separated by delays. They will not all be guaranteed to get the same input or error in some circumstances. It may be fine in the context of CLI only use. But in more generalized use it would benefit from the logic that guaranteed a single reader in PR #3290.

@kzc
Copy link
Contributor

kzc commented Jan 2, 2020

Upon further examination of the stdin plugin I now see that function readStdin() is not exported and will be called just a single time in stdinPlugin().load() which will return the same Promise instance each time. So it should be fine as long as all stdin reading code is using the same plugin.

kzc and others added 9 commits January 4, 2020 20:56
Set default output format to "es" to save CLI typing for the most common case.

closes #1440
closes #3276
* Only allow '-' to use stdin with the JS API
* Use locally installed shx
* Add test for stdin read error
* Move stdin logic to CLI
* Only use stdin as input by default when there is no config file
* Override input with stdin with --stdin CLI flag
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.

Input from string/input stream Feature: accept stdin
2 participants