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

Fix react native server #7187

Merged
merged 10 commits into from
Jun 29, 2019
Merged

Fix react native server #7187

merged 10 commits into from
Jun 29, 2019

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented Jun 25, 2019

Issue: #4716

What I did

Updated events to use storyId.
Removed some of the local state, now we depend on selection inside storyStore.
Stopped using legacy StoryStore api.

Now we are listening for STORY_RENDER event, which allows us to more easily sync between web and mobile.

How to test

Manual testing :(

I am having issues setting up dev env for storybook, so I couldn't run git hooks, sorry.

@vercel
Copy link

vercel bot commented Jun 25, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fix-react-native-server.storybook.now.sh

@@ -89,7 +89,12 @@ export default class StoryStore extends EventEmitter {
this._selection = data === undefined ? this._selection : { storyId, viewMode };
this._error = error === undefined ? this._error : error;

setTimeout(() => this.emit(Events.STORY_RENDER), 1);
setTimeout(() => {
if (this._channel) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ndelangen, I am not sure what kind of implications this could have.

Copy link
Member

Choose a reason for hiding this comment

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

Should be fine, we want to migrate to using the channel for this anyway, might be good to add a code-comment there to know which is the preferred way.

@@ -89,7 +89,12 @@ export default class StoryStore extends EventEmitter {
this._selection = data === undefined ? this._selection : { storyId, viewMode };
this._error = error === undefined ? this._error : error;

setTimeout(() => this.emit(Events.STORY_RENDER), 1);
setTimeout(() => {
if (this._channel) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be fine, we want to migrate to using the channel for this anyway, might be good to add a code-comment there to know which is the preferred way.

@ndelangen ndelangen added this to the 5.1.x milestone Jun 25, 2019
@ndelangen ndelangen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 25, 2019
@benoitdion
Copy link
Member

Awesome cleanup @Gongreg! Added a few comments for things to double check on but LGTM overall.

@vercel vercel bot temporarily deployed to staging June 25, 2019 12:13 Inactive
@ndelangen
Copy link
Member

I'll clean up the formatting

@ndelangen
Copy link
Member

@Gongreg the linting on most react-native code was actively disabled. I enabled it and fixed a ton of linting issues that came up.

If you feel this should be in a separate PR, we could do that.

@Gongreg
Copy link
Member Author

Gongreg commented Jun 26, 2019

It works for me here also. I think that it, this PR is finished :)

@shilman shilman merged commit b3c27f0 into next Jun 29, 2019
@shilman shilman deleted the fix-react-native-server branch July 3, 2019 02:14
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 31, 2019
shilman added a commit that referenced this pull request Jul 31, 2019
Fix react native server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch react-native react-native-server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants