Use the RN packager's host by default #1568

shilman · 2017-08-01T18:16:41Z

What I did

Use the RN packager's host by default as described in #1222

added CRNA example for testing
updated the CLI templates

How to test

First:

yarn && yarn bootstrap

Then test RN:

yarn bootstrap:react-native-vanilla
cd examples/react-native-vanilla
yarn storybook
./node_modules/.bin/react-native run-ios

Then test CNRA:

yarn bootstrap:crna-kitchen-sink
cd examples/crna-kitchen-sink
yarn storybook
yarn ios

oblador · 2017-08-01T18:53:57Z

examples/crna-kitchen-sink/App.js

@@ -0,0 +1,25 @@
+export default from './storybook';
+
+// import React from 'react';


Is this left here intentionally?

Yeah, I'll add to the comment tho. 😄

oblador · 2017-08-01T18:56:23Z

examples/react-native-vanilla/storybook/storybook.js

@@ -10,9 +10,8 @@ configure(() => {

 const StorybookUI = getStorybookUI({
  port: 7007,
-  host: 'localhost',
+  // host: 'localhost',


Should probably be deleted.

oblador · 2017-08-01T18:57:25Z

examples/crna-kitchen-sink/storybook/stories/Welcome/index.js

+  };
+
+  showApp(event) {
+    event.preventDefault();


Forgive my ignorance, but why is preventDefault needed?

copypasta? i'm not sure.

I'm also unsureFWIW

It's all over the code (part of the CLI boilerplate), so I propose we address it separately 😕

You do not typically need to .preventDefault() in React Native. That said I don't see the showApp function used anywhere in this component... so couldn't this method just be removed?

although - this might be something CRNA related so if so, please ignore my thoughts on removing it.

OK OK looks like it isn't used so I'll remove across the board, test, and submit. Thanks 🙇

On third thought, I think this is a separate issue. This used to be used to show off addon-links but was removed at some point. Opened: #1569

codecov · 2017-08-01T19:07:51Z

Codecov Report

Merging #1568 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1568      +/-   ##
==========================================
- Coverage   20.43%   20.43%   -0.01%     
==========================================
  Files         241      241              
  Lines        5250     5252       +2     
  Branches      656      645      -11     
==========================================
  Hits         1073     1073              
- Misses       3661     3682      +21     
+ Partials      516      497      -19

Impacted Files	Coverage Δ
app/react-native/src/preview/index.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/configs/handle_routing.js	`28.94% <0%> (ø)`	⬆️
app/react-native/src/bin/storybook-start.js	`0% <0%> (ø)`	⬆️
addons/notes/src/react.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/KnobManager.js	`35.41% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/routes.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/element_check.js	`41.17% <0%> (ø)`	⬆️
app/react/src/client/preview/error_display.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/markdown/htags.js	`30% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/libs/filters.js	`53.33% <0%> (ø)`	⬆️
... and 10 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 7cdc25a...67a7093. Read the comment docs.

orta

I ran the same code in our non-CRNA app, and everything was good:

import { NativeModules } from "react-native"
console.log(NativeModules.SourceCode.scriptURL)

And got http://localhost:8081/Example/Emission/index.ios.bundle?platform=ios&dev=true&hot=true

So this is fine for non-CRNA too 👍

atticoos · 2017-08-01T19:41:11Z

app/react-native/src/preview/index.js

@@ -55,8 +57,7 @@ export default class Preview {
      let webUrl = null;
      let channel = addons.getChannel();
      if (params.resetStorybook || !channel) {
-        const host = params.host || 'localhost';
-
+        const host = params.host || parse(NativeModules.SourceCode.scriptURL).hostname;


Brilliant - didn’t know this was an accessible thing

atticoos · 2017-08-01T19:43:09Z

app/react-native/src/preview/index.js

@@ -55,8 +57,7 @@ export default class Preview {
      let webUrl = null;
      let channel = addons.getChannel();
      if (params.resetStorybook || !channel) {
-        const host = params.host || 'localhost';
-
+        const host = params.host || parse(NativeModules.SourceCode.scriptURL).hostname;
        const port = params.port !== false ? `:${params.port || 7007}` : '';


Is this the port for the ReactNative packager? Or where storybooks web previewed will be binding to?

this is the storybook port

shilman added 2 commits August 1, 2017 11:12

Add CRNA kitchen sink example for testin

aebecbe

Use RN packager host by default in examples and also CLI templates

8f10fce

shilman changed the title ~~1222 parse rn packager host~~ Use the RN packager's host by default Aug 1, 2017

shilman requested review from orta, tmeasday, atticoos and rmevans9 August 1, 2017 18:17

shilman mentioned this pull request Aug 1, 2017

Stories doesn't show up in the browser #1398

Closed

oblador reviewed Aug 1, 2017

View reviewed changes

Updated RN packager host with PR comments

279dc89

shilman added feature request react-native labels Aug 1, 2017

orta approved these changes Aug 1, 2017

View reviewed changes

shilman mentioned this pull request Aug 1, 2017

Add addon-link example to react-native #1569

Closed

3 tasks

Merge branch 'master' into 1222-parse-rn-packager-host

67a7093

shilman merged commit d7bc10d into master Aug 1, 2017

shilman deleted the 1222-parse-rn-packager-host branch August 1, 2017 19:41

atticoos reviewed Aug 1, 2017

View reviewed changes

D1no mentioned this pull request Aug 28, 2017

RN: Unable to resolve module AccessibilityInfo #1503

Closed

fjtrujy mentioned this pull request Jan 31, 2019

Bundled Android application is not connecting to Web Socket server #5435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the RN packager's host by default #1568

Use the RN packager's host by default #1568

shilman commented Aug 1, 2017

oblador Aug 1, 2017

shilman Aug 1, 2017

oblador Aug 1, 2017

oblador Aug 1, 2017

shilman Aug 1, 2017

orta Aug 1, 2017

shilman Aug 1, 2017 •

edited

Loading

rmevans9 Aug 1, 2017

rmevans9 Aug 1, 2017

shilman Aug 1, 2017

shilman Aug 1, 2017

codecov bot commented Aug 1, 2017 •

edited

Loading

orta left a comment

atticoos Aug 1, 2017

atticoos Aug 1, 2017

shilman Aug 1, 2017

		@@ -0,0 +1,25 @@
		export default from './storybook';

		// import React from 'react';

Use the RN packager's host by default #1568

Use the RN packager's host by default #1568

Conversation

shilman commented Aug 1, 2017

What I did

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 1, 2017 • edited Loading

Codecov Report

orta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman Aug 1, 2017 •

edited

Loading

codecov bot commented Aug 1, 2017 •

edited

Loading