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

Updates storybook-start.js to use child_process instead of shelljs #3527

Merged
merged 3 commits into from
May 29, 2018

Conversation

sky-franciscogoncalves
Copy link
Contributor

Issue

ERROR  Error running yarn run storybook -- --config haul.config.js --platform all
Error: Given stream is not a TTY
at makeTTYAdapter (node_modules/react-stream-renderer/src/adapters/TTY/makeTTYAdapter.js:13:11)
at initUI (node_modules/haul/src/server/ui.js:72:9)
at createServer (node_modules/haul/src/server/index.js:54:28)
at Object.start [as action] (node_modules/haul/src/commands/start.js:51:3)
at <anonymous>
at process._tickCallback (internal/process/next_tick.js:188:7)
at Function.Module.runMain (module.js:678:11)
at startup (bootstrap_node.js:187:16)
at bootstrap_node.js:608:3

What I did

Updated react-native's storybook-start.js to use child_process instead of ShellJS. This is because the later does not support running commands that require interactive input, as you can see here.

The need to use these type of commands, started when I updated both Haul and Storybook to their last versions.

How to test

To test this you should update Haul to v1.0.0-rc.0 and Storybook to v4.0.0-alpha.4. Then you should perform storybook start -p 7007 --haul haul.config.js --platform all. Where the config file follows Haul's new configuration style.

Please let me know if I can improve this PR in anyway.

@@ -1,9 +1,9 @@
#!/usr/bin/env node
/* eslint-disable no-console */

/* eslint-disable-next-line camelcase */
import child_process from 'child_process';
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you don't like camelCase?

Copy link
Member

Choose a reason for hiding this comment

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

We like camelCase so much we lint in favour of it?

You could possibly import like:

import { spawn } from 'child_process';

I think that should make the warning go away?

Copy link
Member

Choose a reason for hiding this comment

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

@sky-franciscogoncalves Do you think you could fix this minor comment?, and we can get this in the next major release!

@codecov
Copy link

codecov bot commented May 29, 2018

Codecov Report

Merging #3527 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3527   +/-   ##
=======================================
  Coverage   37.61%   37.61%           
=======================================
  Files         459      459           
  Lines       10377    10377           
  Branches      928      922    -6     
=======================================
  Hits         3903     3903           
+ Misses       5932     5903   -29     
- Partials      542      571   +29
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-start.js 0% <ø> (ø) ⬆️
addons/info/src/components/markdown/pre/pre.js 77.58% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 7.04% <0%> (ø) ⬆️
addons/storyshots/src/rn/loader.js 53.33% <0%> (ø) ⬆️
app/react-native/src/bin/storybook.js 0% <0%> (ø) ⬆️
addons/jest/src/components/PanelTitle.js 0% <0%> (ø) ⬆️
...ons/actions/src/lib/types/object/configureDepth.js 62.5% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 51.85% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
... and 61 more

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 3539625...d67752c. Read the comment docs.

@wuweiweiwu
Copy link
Member

wuweiweiwu commented May 29, 2018

Thanks for contributing @sky-franciscogoncalves 👍

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

Successfully merging this pull request may close these issues.

3 participants