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

React native fails with JSON.serialization issue #13

Closed
zhigang1992 opened this issue Jan 2, 2020 · 27 comments
Closed

React native fails with JSON.serialization issue #13

zhigang1992 opened this issue Jan 2, 2020 · 27 comments
Labels
bug Something isn't working has PR high priority

Comments

@zhigang1992
Copy link

Describe the bug
Default configuration with sb stopped working after beta.25

To Reproduce
Steps to reproduce the behavior:

  1. npx react-native init testSB --template react-native-template-typescript
  2. npx -p @storybook/cli sb init --type react_native
  3. Launch the app
  4. Launch the storybook server

Expected behavior
Should not throw error

Screenshots
Simulator Screen Shot - iPhone 11 - 2020-01-02 at 10 32 25

System:

Environment Info:

  System:
    OS: macOS 10.15.2
    CPU: (8) x64 Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz
  Binaries:
    Node: 10.16.3 - /var/folders/xx/4pg__j5s58585b86mzz27z540000gn/T/fnm-shell-1707442/bin/node
    Yarn: 1.21.1 - ~/.yarn/bin/yarn
    npm: 6.9.0 - /var/folders/xx/4pg__j5s58585b86mzz27z540000gn/T/fnm-shell-1707442/bin/npm
  Browsers:
    Chrome: 79.0.3945.88
    Safari: 13.0.4
  npmPackages:
    @storybook/addon-actions: 5.3.0-beta.25 => 5.3.0-beta.25
    @storybook/addon-links: 5.3.0-beta.25 => 5.3.0-beta.25
    @storybook/addons: 5.3.0-beta.25 => 5.3.0-beta.25
    @storybook/react-native: 5.3.0-beta.25 => 5.3.0-beta.25
    @storybook/react-native-server: 5.3.0-beta.25 => 5.3.0-beta.25

Additional context
It still works after downgrade to beta.25 and it stops working after beta.26 and up.

@doctyper
Copy link

doctyper commented Jan 4, 2020

I just ran into this issue as well.

The error comes from the use of json-fn, which does not guard against circular references in JSON objects.

There is a circular reference in hooks.ts. context includes a reference to hooks, which has a parameter currentContext, which itself includes a reference to hooks, hence the circular ref.

I'm using the following patch to workaround the issue:

diff --git a/node_modules/@storybook/addons/dist/hooks.js b/node_modules/@storybook/addons/dist/hooks.js
index 6c88e84..8ad6678 100644
--- a/node_modules/@storybook/addons/dist/hooks.js
+++ b/node_modules/@storybook/addons/dist/hooks.js
@@ -264,7 +264,7 @@ var applyHooks = function applyHooks(applyDecorators) {
       var hooks = context.hooks;
       hooks.prevMountedDecorators = hooks.mountedDecorators;
       hooks.mountedDecorators = new Set([getStory].concat(_toConsumableArray(decorators)));
-      hooks.currentContext = context;
+      hooks.currentContext = Object.assign({}, context, { hooks: null });
       hooks.hasUpdates = false;
       var result = decorated(context);
       numberOfRenders = 1;

@shilman
Copy link
Member

shilman commented Jan 5, 2020

cc @Hypnosphi can we patch this if it makes sense?

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 5, 2020

Unfortunately, the proposed change doesn't support mutation of context obtained from useStoryContext:

const context = useStoryContext()
context.foo = 'bar'

As an alternative, we can use telejson as we do in channel-postmessage

@doctyper
Copy link

doctyper commented Jan 6, 2020

I will of course defer to you, but I would suggest exposing a setter API as opposed to encouraging a direct mutation:

const [context, setContext] = useContext()

...

setContext({ foo: ‘bar’ })

This would support my workaround. But again, I will defer to your judgement.

@rdias-dev
Copy link

+1

@mi5ha
Copy link

mi5ha commented Jan 25, 2020

If I am not mistaken, current version of Storybook is unusable until this is solved right?

Is there some older version of Storybook that doesnt have this bug?

@zhigang1992
Copy link
Author

Default configuration with sb stopped working after beta.25

@mi5ha

@QuinnRhys89
Copy link

Dealing with this issue as well. Any traction on this?

@TGTGamer
Copy link

+1 Same issue, real shame as this tool looks great. Guess while it's unfixed i'll either have to revert or continue without storybook for a bit

@arlyon
Copy link

arlyon commented Jan 31, 2020

I just ran into this issue as well.

The error comes from the use of json-fn, which does not guard against circular references in JSON objects.

There is a circular reference in hooks.ts. context includes a reference to hooks, which has a parameter currentContext, which itself includes a reference to hooks, hence the circular ref.

I'm using the following patch to workaround the issue:

diff --git a/node_modules/@storybook/addons/dist/hooks.js b/node_modules/@storybook/addons/dist/hooks.js
index 6c88e84..8ad6678 100644
--- a/node_modules/@storybook/addons/dist/hooks.js
+++ b/node_modules/@storybook/addons/dist/hooks.js
@@ -264,7 +264,7 @@ var applyHooks = function applyHooks(applyDecorators) {
       var hooks = context.hooks;
       hooks.prevMountedDecorators = hooks.mountedDecorators;
       hooks.mountedDecorators = new Set([getStory].concat(_toConsumableArray(decorators)));
-      hooks.currentContext = context;
+      hooks.currentContext = Object.assign({}, context, { hooks: null });
       hooks.hasUpdates = false;
       var result = decorated(context);
       numberOfRenders = 1;

I can confirm that this fix 'works' but can be subtly buggy. For my purposes it is "good enough" to tide me over.

@brotsky
Copy link

brotsky commented Jan 31, 2020

@arlyon Thanks for posting that diff! It worked for me!

Hopefully, the next version fixes this issue. This has been a huge headache for me!

@shilman shilman transferred this issue from storybookjs/storybook Feb 2, 2020
@jenskuhrjorgensen
Copy link

jenskuhrjorgensen commented Feb 3, 2020

Thx @doctyper - it works for me too!

My 5 cents to folks who are not familiar with patches. To apply the suggested patch (with npm):

  1. npm i patch-package
  2. Add script to package.json to apply the patch after every install: "postinstall": "patch-package
  3. Replace
    hooks.currentContext = context;
    with
    hooks.currentContext = Object.assign({}, context, { hooks: null });
    in the file
    node_modules/@storybook/addons/dist/hooks.js
  4. Run npx patch-package @storybook/addons
  5. A file named @storybook+addons+{VERSION}.patch in the patches folder will be created and applied every time you run npm install

@simonepizzamiglio
Copy link

simonepizzamiglio commented Feb 4, 2020

Thx @doctyper - it works for me too!

My 5 cents to folks who are not familiar with patches. To apply the suggested patch:

  1. Replace
    hooks.currentContext = context;
    with
    hooks.currentContext = Object.assign({}, context, { hooks: null });
    in the file
    node_modules/@storybook/addons/dist/hooks.js
  2. Run npx patch-package @storybook/addons
  3. A file named @storybook+addons+{VERSION}.patch in the patches folder will be created and applied every time you run npm install

@jenskuhrjorgensen you still need to install patch-package in your dependencies though?

@davidnuvolar
Copy link

davidnuvolar commented Feb 4, 2020

Thx @doctyper - it works for me too!
My 5 cents to folks who are not familiar with patches. To apply the suggested patch:

  1. Replace
    hooks.currentContext = context;
    with
    hooks.currentContext = Object.assign({}, context, { hooks: null });
    in the file
    node_modules/@storybook/addons/dist/hooks.js
  2. Run npx patch-package @storybook/addons
  3. A file named @storybook+addons+{VERSION}.patch in the patches folder will be created and applied every time you run npm install

@jenskuhrjorgensen you still need to install patch-package in your dependencies though?

@simonepizzamiglio you should add it as a dependency and then, in your package.json scripts phase add the following:
"scripts": { ... "postinstall": "patch-package", ... }
This way, every time you run a yarn install, the patch will be applied afterwards.

@jenskuhrjorgensen
Copy link

You are absolutely right, @simonepizzamiglio and @davidnuvolar. I've updated my previous comment - thanks!

@Gongreg Gongreg added bug Something isn't working high priority labels Feb 5, 2020
@brotsky
Copy link

brotsky commented Feb 5, 2020

Thanks @jenskuhrjorgensen for the explanation of how to create patches! That worked perfectly for me!

@brian-perlego
Copy link

Any update on this issue ?

@Gongreg
Copy link
Member

Gongreg commented Feb 11, 2020

Hey, sadly not yet. But this is on my todo list for this week.

@ghost
Copy link

ghost commented Feb 12, 2020

This also happened to me with @storybook/[email protected] and the patch workaround works for now

@Gongreg
Copy link
Member

Gongreg commented Feb 15, 2020

Hey, I've created a PR to fix it in the main storybook repo.

We still are in the middle of migrating out React Native from Storybook monorepo, so I can't promise when exactly the fix will be available, but I will try to get it out as soon as I can.

I am sorry that RN Storybook was abandoned for a while. Soon RN Storybook is going to be stable (that is why we are moving it out of main monorepo) and issues like these should stop popping up.

@Gongreg Gongreg added the has PR label Feb 15, 2020
@jonathanmachado
Copy link

Sorry I need this fix please.

Thanks

@jonathanmachado
Copy link

@Gongreg please you can check this thanks

@davidnuvolar
Copy link

@Gongreg please you can check this thanks

Hi @jonathanmachado ,
if you need this fixed urgently, there is already a working patch in this thread, posted by jenskuhrjorgensen.

@jonathanmachado
Copy link

Thanks @davidnuvolar it's working with this fix, only one question this fix remove actions logger? because in my stories I don't see actions event loggs, Thanks again

@brian-perlego
Copy link

Storybook 5.3.14 fixed the issue for me.
Thanks @Gongreg !

@davidnuvolar
Copy link

I confirm the fix is working in 5.3.14. Thank you @Gongreg !

@Gongreg
Copy link
Member

Gongreg commented Mar 6, 2020

Great! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has PR high priority
Projects
None yet
Development

No branches or pull requests