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

storybook on react-native does not work with hmr (hot module reload) properly #2081

Closed
macrozone opened this issue Oct 18, 2017 · 17 comments
Closed

Comments

@macrozone
Copy link
Contributor

macrozone commented Oct 18, 2017

For HMR (hot module reload) to work, the root component has to be a class, as it seems.

I therefore added this to storybook/storybook.js :

const StorybookUIOrg = getStorybookUI({ port: 7007, onDeviceUI: true })

const StorybookUI = class extends Component {
  render() {
    return <StorybookUIOrg />
  }
}

AppRegistry.registerComponent('homestory', () => StorybookUI)

This used to work in the past, but with latest version, i just see the native ui menu and some warnings like:

"The prop url is marked as required in StoryView but it's value is null"

and

"The prop url is marked as required in OnDeviceUI, but it's value is null"

Edit: without the class above, there is no warning. But hot reload does not work. On the simulator it notifies about "hot reloading", but nothing changes.

Edit 2: if i change to a different story after hot reload and back, the changes are applied... So what's wrong?

I now switched back to live reload, HMR seems broken at the time beeing

@danielduan
Copy link
Member

danielduan commented Oct 30, 2017

The propType bug is related to #2117

danielduan added a commit that referenced this issue Oct 30, 2017
@danielduan danielduan self-assigned this Oct 31, 2017
@danielduan danielduan changed the title storybook on react-native does not work with hmr properly storybook on react-native does not work with hmr (hot module reload) properly Oct 31, 2017
danielduan added a commit that referenced this issue Oct 31, 2017
Hypnosphi added a commit that referenced this issue Nov 1, 2017
#2081 fix hmr in react-native template
@macrozone
Copy link
Contributor Author

macrozone commented Nov 2, 2017

@danielduan The issue is not resolved. I updated storybook to yesterdays version (3.2.14) and verified that the storybook index file matches this template #2194 (I already had the base component as a class instead of a stateless function).

HMR still does not work in this setup. Any change to a file will trigger a refresh in the app, but it will crash with:

console.error: "(node) warning: possible EventEmitter memory leak detected. %d listeners added. Use emitter.setMaxListeners() to increase limit.", 11

Object.console.error
    hashAssetFiles:55868:11
EventEmitter.addListener
    hashAssetFiles:166001:15
 (...)

by using the debugger i see that this happens in

 _this3._events.on('setCurrentStory', function (d) {
          return _this3._selectStory(d);
        });

Edit:

I am using CRNA (with expo) and i have added decorators to stories (e.g. ThemeProvider from styled-components).

Edit2: If I dismiss the error, storybook will show the startscreen again for a moment and go back to the story with the changes. Any further change to the components will have no effect and won't trigger HMR anymore.

Edit3: if i disable remote debugging, the memory leak error is still there, but otherwise HMR seems to work besides this bug: #835

@danielduan danielduan reopened this Nov 2, 2017
@danielduan
Copy link
Member

danielduan commented Nov 2, 2017

The decorators themselves might break HMR if it's a stateless functional component. Our event emitter probably needs some work too.

If you have time to dig through this, we'd love some help on a PR.

danielduan added a commit that referenced this issue Nov 2, 2017
#2081 add hmr to other rn app template
@danielduan
Copy link
Member

Do you have a repo so I can try to reproduce this? @macrozone

I just tried in our CRNA example and it seems to work:

7:14:27 PM: [React Transform HMR] Patching Welcome

7:14:45 PM: [React Transform HMR] Patching Welcome

@Maxim-Filimonov
Copy link

Maxim-Filimonov commented Feb 3, 2018

I found this issue while trying to figure out why 3.3.12 is not working with HMR.
It does print in the console as expected, however the screen on the device doesn't get updated.
See the video of the issue. Repository is fresh from GitHub - bootstraped using yarn bootstrap.


Update
After playing around a bit found that the issue is actually only happens with static fields inside class...
Thinking about it, it does make sense that it doesn't work as it essentially patches constructor which won't be executed unless component is remounted.
Found that moving styles into separate file fixes the problem. Hopefully, it will help people facing similar issue to diagnose it.

@macrozone
Copy link
Contributor Author

it's hard to tell what's wrong with hmr and react-native. It works for some time, but starts to get slower and slower, eats cpu on both the device and the computer, throwing errors like the dreaded "possible event emitter leaked" and now kills the server eventually (see #2923)

So it basically "works" but extremly unreliable and i therefore wonder if anyone successfully uses it on real projects

@axelnormand
Copy link

axelnormand commented Mar 9, 2018

I too am getting this error.
For now I've added this hack to my init storybook function to at least delay the error for a bit :)
I'll report back if it works or not.

require('events').EventEmitter.defaultMaxListeners = 200;

I'm using a global decorator along with create react native app, typescript and storyshots.
Happy to attempt a PR if anyone can suggest the best place to look

@jqn
Copy link

jqn commented Nov 8, 2018

I'm on "@storybook/react-native": "^4.0.0-alpha.25" and in order to get hot reloading working again after integrating storybook into my app I had to do the following.
Note the comment about react-native hot module loader must take a class.

/**
 * storybook/index.js
 **/
import React, { Component } from "react";
import { AppRegistry } from "react-native";
import { getStorybookUI, configure } from "@storybook/react-native";

import "./rn-addons";

// import stories
configure(() => {
  require("../App/Components/Stories");
}, module);

// Refer to https://github.com/storybooks/storybook/tree/master/app/react-native#start-command-parameters
// To find allowed options for getStorybookUI
const StorybookUIRoot = getStorybookUI({ port: 7007, onDeviceUI: true });

// react-native hot module loader must take in a Class - https://github.com/facebook/react-native/issues/10991
// https://github.com/storybooks/storybook/issues/2081
// eslint-disable-next-line react/prefer-stateless-function
class StorybookUIHMRRoot extends Component {
  render() {
    return <StorybookUIRoot />;
  }
}

// If you are using React Native vanilla and after installation you don't see your app name here, write it manually.
// If you use Expo you can safely remove this line.
// AppRegistry.registerComponent("ExportPro", () => StorybookUIRoot);

export default StorybookUIHMRRoot;

@Gongreg
Copy link
Member

Gongreg commented Nov 9, 2018

Hey @jqn, so to be clear you had to create StorybookUIHMRRoot even though we already create it in getStorybookUI?

@jqn
Copy link

jqn commented Nov 12, 2018

Yes I did. The hot reload was broken with the default install configuration.

@jjm340
Copy link

jjm340 commented Feb 14, 2019

@jqn This worked for me as well

@shilman shilman closed this as completed Mar 22, 2019
@shilman shilman reopened this Mar 22, 2019
@shilman
Copy link
Member

shilman commented Mar 22, 2019

We’ve released a brand new @storybook/react-native with a bunch of core improvements. It’s available in the latest 5.1-alpha on next and has been verified by several RN users on their existing apps. It should fix a bunch of compatibility issues, especially if you’re using the web server feature. Please give it a try and comment here if it fixes your problem. Migration instructions available here: https://github.com/storybooks/storybook/blob/next/MIGRATION.md#react-native-server

@macrozone @odino @b3ngineer @maxhungry @tonyxiao @Bardiamist @danielduan @EskelCz @Maxim-Filimonov @axelnormand @jqn @jjm340 @iamolegga @FunkSoulNinja @miltoneiji @Gongreg

@stereodenis
Copy link
Contributor

Still not working on 5.1 for me

@shilman
Copy link
Member

shilman commented Jul 25, 2019

@stereodenis try 5.2-beta? 🙈

@stereodenis
Copy link
Contributor

@shilman 5.2-beta.7
image

@Gongreg
Copy link
Member

Gongreg commented Jul 25, 2019

Sadly there was no effort done to fix hmr

@Gongreg
Copy link
Member

Gongreg commented Nov 2, 2019

React Native 0.61 has a new reload format, so closing this issue. The new issue is #7916

@Gongreg Gongreg closed this as completed Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants