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

Addon Actions is not handling React events properly #6471

Closed
vdh opened this issue Apr 10, 2019 · 33 comments
Closed

Addon Actions is not handling React events properly #6471

vdh opened this issue Apr 10, 2019 · 33 comments

Comments

@vdh
Copy link
Contributor

vdh commented Apr 10, 2019

Describe the bug
Attempting to use action from @storybook/addon-actions causes console errors and the output doesn't match the docs, possibly due to how the React Synthetic Events are being serialized.

To Reproduce

create-react-app sbtemp
cd sbtemp
npx -p @storybook/cli sb init
yarn storybook

Then the demo button in the "Button" "with Text" story is clicked. Google Chrome gives a deprecation warning:

[Deprecation] 'window.webkitStorageInfo' is deprecated. Please use 'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage' instead.

The output in the Storybook Actions addon panel doesn't match the output shown in the docs, reading as clicked: [Class] instead of the clicked: ["[SyntheticEvent]", null, "[SyntheticEvent]"] shown on https://storybook.js.org/docs/addons/introduction/

clicked: [Class]
0: Class
_dispatchInstances: Object
_dispatchListeners: function action()
_targetInst: Object
altKey: false
bubbles: true
button: 0
buttons: 0
cancelable: true
clientX: 82
clientY: 32
ctrlKey: false
currentTarget: HTMLButtonElement
defaultPrevented: false
detail: 1
dispatchConfig: Object
eventPhase: 3
getModifierState: function modifierStateGetter()
isDefaultPrevented: function functionThatReturnsFalse()
isPropagationStopped: function functionThatReturnsFalse()
isTrusted: true
metaKey: false
movementX: 0
movementY: 0
nativeEvent: MouseEvent
pageX: 82
pageY: 32
relatedTarget: null
screenX: 292
screenY: 185
shiftKey: false
target: HTMLButtonElement
timeStamp: 266833.9550000001
type: "click"
view: Window

Additionally, if react and react-dom is downgraded to 16.3.2 (the version I was using when I encountered the issue), there is also another console error:

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're adding a new property in the synthetic event object. The property is never released. See https://fb.me/react-event-pooling for more information.

Expected behavior
No console errors or warnings, and for the Action panel to match the docs.

System:

  • OS: macOS Mojave Version 10.14.4
  • Device: MacBook Pro
  • Browser: Google Chrome Version 73.0.3683.103 (Official Build) (64-bit)
  • Framework: react
  • Addons: addon-actions
  • Version:
    react@^16.8.6
    react-dom@^16.8.6
    react-scripts@2.1.8
    @storybook/react@^5.0.6
    @storybook/addon-actions@^5.0.6
    @storybook/addon-links@^5.0.6
    @storybook/addons@^5.0.6
    @babel/core@^7.4.3
    babel-loader@^8.0.5

Additional context
I was unable to recreate this additional error, but in my own setup I also saw telejson / JSON.stringify choke on something (Date-related?) with an error of Uncaught TypeError: toISOString is not a function

@vdh vdh changed the title a is not handling React events properly Addon Actions is not handling React events properly Apr 10, 2019
@wonskarol
Copy link

wonskarol commented Apr 23, 2019

Im facing similar issue.

@shilman
Copy link
Member

shilman commented Apr 23, 2019

Sounds like something weird is going on. Any chance you can provide a repro?

@shilman shilman added this to the 5.0.x milestone Apr 23, 2019
@vdh
Copy link
Contributor Author

vdh commented Apr 24, 2019

@shilman I gave some example CLI commands to reproduce it, is there a better repro format or service to use?

I tested it again just now and got the same issue (Chrome 73.0.3683.103, Storybook 5.0.10, Node v11.14.0)

@shilman
Copy link
Member

shilman commented Apr 24, 2019

@vdh Sorry I missed that. I'll check it out!

@shilman
Copy link
Member

shilman commented Apr 24, 2019

Ok, I can reproduce. Marking this as a bug & will gratefully accept any PRs to fix! 🙇

@kohakukun
Copy link
Contributor

Aside from the warning, what would be the desired behavior?

@vdh
Copy link
Contributor Author

vdh commented Apr 29, 2019

@kohakukun Not causing warnings/errors, and consistent behaviour with what is described by the docs. So either update the behaviour to match the docs or vice versa. Compatibility with React 16.3.2 would be nice but I would understand if that's not possible. (I've updated React so this is no longer relevant)

I assume the Uncaught TypeError: toISOString is not a function error I was encountering was either fallout from the event handling issue or a problem in my own code, which should hopefully be fixed once the main issue is addressed.

@mousemke
Copy link

mousemke commented May 6, 2019

i'm actually hitting the Uncaught TypeError: toISOString is not a function after update, and cannot find an issue aside from this issue, so i'm hoping it is not your code and is instead part of this issue.

@alechp
Copy link

alechp commented May 17, 2019

I'm using @storybook/react-native and @storybook/react-native-server and having the same issue as described above.

I'm also getting the synthetic event is used for performance reasons error described above. I haven't updated react-dom.

Source (storybook/stories/index.js)

storiesOf("Button", module)
  .addDecorator(getStory => <CenterView>{getStory()}</CenterView>)
  .add("with text", () => (
    <Button onPress={action("clicked-text")}>
      <Text>Hello Button</Text>
    </Button>
  ))

Error

The error below recurses

Screenshot

Screen Shot 2019-05-17 at 11 01 05 AM

Text

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're %s `%s` on a released/nullified synthetic event. %s. If you must keep the original synthetic event around, use event.persist(). See https://fb.me/react-event-pooling for more information., accessing the property, target, This is set to null
- node_modules/react-native/Libraries/Renderer/oss/ReactNativeRenderer-dev.js:619:8 in warningWithoutStack
- node_modules/react-native/Libraries/Renderer/oss/ReactNativeRenderer-dev.js:1577:10 in warn
- node_modules/react-native/Libraries/Renderer/oss/ReactNativeRenderer-dev.js:1569:9 in get$$1
* [native code]:null in stringify
- node_modules/@storybook/react-native/node_modules/@storybook/channel-websocket/dist/index.js:67:46 in sendNow
- node_modules/@storybook/react-native/node_modules/@storybook/channel-websocket/dist/index.js:56:21 in send
- node_modules/@storybook/channels/dist/index.js:126:32 in handler
- node_modules/react-native/Libraries/Core/Timers/JSTimers.js:152:14 in _callTimer
- node_modules/react-native/Libraries/Core/Timers/JSTimers.js:200:17 in _callImmediatesPass
- node_modules/react-native/Libraries/Core/Timers/JSTimers.js:464:30 in callImmediates
* [native code]:null in callImmediates
- node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:320:6 in __callImmediates
- node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:135:6 in <unknown>
- node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:297:10 in __guard
- node_modules/react-native/Libraries/BatchedBridge/MessageQueue.js:134:17 in flushedQueue
* [native code]:null in flushedQueue
* [native code]:null in callFunctionReturnFlushedQueue

Screenshots

Chrome (@storybook/react-native-server)

Screen Shot 2019-05-17 at 11 00 44 AM

iOS Simulator (@storybook/react-native)

Screen Shot 2019-05-17 at 11 06 06 AM

Screen Shot 2019-05-17 at 11 04 03 AM

Config

package.json

  "scripts": {
    "android": "expo start --android",
    "build": "yarn run build:components",
    "build:components": "webpack --config ./webpack.components.js",
    "eject": "expo eject",
    "expo": "expo start",
    "ios": "expo start --ios",
    "start": "npm-run-all --parallel expo storybook",
    "storybook": "start-storybook -p 7007"
  },
  "dependencies": {
    "@emotion/native": "^10.0.11",
    "expo": "^32.0.0",
    "native-base": "^2.12.1",
    "react": "16.5.0",
    "react-native": "https://github.com/expo/react-native/archive/sdk-32.0.0.tar.gz",
    "react-router": "^5.0.0"
  },
  "devDependencies": {
    "@babel/preset-react": "^7.0.0",
    "@emotion/core": "^10.0.10",
    "@storybook/addon-actions": "^5.0.11",
    "@storybook/addon-links": "^5.0.11",
    "@storybook/addon-storysource": "^5.0.11",
    "@storybook/addons": "^5.0.11",
    "@storybook/react-native": "^5.0.11",
    "@storybook/react-native-server": "5.1.0-alpha.7",
    "babel-core": "^6.26.3",
    "babel-loader": "^8.0.5",
    "babel-preset-expo": "^5.0.0",
    "babel-runtime": "^6.26.0",
    "duplicate-package-checker-webpack-plugin": "^3.0.0",
    "emotion-theming": "^10.0.10",
    "npm-run-all": "^4.1.5",
    "prop-types": "^15.7.2",
    "react-dom": "16.5.0",
    "size-plugin": "^1.2.0",
    "storybook-react-router": "^1.0.5",
    "webpack-cli": "^3.3.2",
    "webpack-node-externals": "^1.7.2"
  },

Addon files

addons.js

import "@storybook/addon-actions/register";
import "@storybook/addon-links/register";
import "@storybook/addon-storysource/register";

rn-addons.js

import "@storybook/addon-ondevice-actions/register";
import "@storybook/addon-ondevice-links/register";
import "@storybook/addon-ondevice-storysource/register";

@shilman
Copy link
Member

shilman commented May 19, 2019

@alechp Can you open a separate issue? ondevice-actions is a RN addon, and I'd like the RN guys to look at it, but I'm not sure it's the same issue.

@alechp
Copy link

alechp commented May 19, 2019

@shilman Roger. New issue created here: https://github.com/storybooks/storybook/issues/6827

@ndelangen ndelangen modified the milestones: 5.0.x, 5.1.x Jun 3, 2019
@umakantp
Copy link

umakantp commented Jun 8, 2019

I'm getting the following error when I listen to click even on a button and pass it to actions:-

_ctx.js:18 Uncaught TypeError: toISOString is not a function
    at String.toJSON (<anonymous>)
    at Object.<anonymous> (_ctx.js:18)
    at JSON.stringify (<anonymous>)
    at Object.stringify (index.js:353)
    at PostmsgTransport.push../node_modules/@storybook/channel-postmessage/dist/index.js.PostmsgTransport.send (index.js:43)
    at handler (index.js:46)
    at Channel.push../node_modules/@storybook/channels/dist/index.js.Channel.emit (index.js:55)
    at action (action.js:32)
    at HTMLUnknownElement.callCallback (react-dom.development.js:149)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:199)
(anonymous) @ _ctx.js:18
stringify @ index.js:353
push../node_modules/@storybook/channel-postmessage/dist/index.js.PostmsgTransport.send @ index.js:43
handler @ index.js:46
push../node_modules/@storybook/channels/dist/index.js.Channel.emit @ index.js:55
action @ action.js:32
callCallback @ react-dom.development.js:149
invokeGuardedCallbackDev @ react-dom.development.js:199
invokeGuardedCallback @ react-dom.development.js:256
invokeGuardedCallbackAndCatchFirstError @ react-dom.development.js:270
executeDispatch @ react-dom.development.js:561
executeDispatchesInOrder @ react-dom.development.js:583
executeDispatchesAndRelease @ react-dom.development.js:680
executeDispatchesAndReleaseTopLevel @ react-dom.development.js:688
forEachAccumulated @ react-dom.development.js:662
runEventsInBatch @ react-dom.development.js:816
runExtractedEventsInBatch @ react-dom.development.js:824
handleTopLevel @ react-dom.development.js:4826
batchedUpdates$1 @ react-dom.development.js:20439
batchedUpdates @ react-dom.development.js:2151
dispatchEvent @ react-dom.development.js:4905
(anonymous) @ react-dom.development.js:20490
unstable_runWithPriority @ scheduler.development.js:255
interactiveUpdates$1 @ react-dom.development.js:20489
interactiveUpdates @ react-dom.development.js:2170
dispatchInteractiveEvent @ react-dom.development.js:4882

My code is as simple as
<Button onClick={actions('clicked')}Default</Button>

Button component accepts the onClick and prop and sets on <input type="button" onClick={props.onClick} />.

Versions are following:-

    "@storybook/addon-actions": "5.0.11",
    "@storybook/addon-links": "5.0.11",
    "@storybook/addons": "5.0.11",
    "@storybook/react": "5.0.11",

@mousemke
Copy link

i just wanted to pass this on. we have been having this issue for a while now (in JS, storybook-react)

we fixed it with a small tweak to how we called actions. it seemed to be an issue of how the addon was serializing the date. maybe this just applies to us, but also maybe it'll help some folk

fixed.

Screenshot 2019-06-12 at 17 44 00

@montezume
Copy link

I'm also having this issue with the errors / warnings about reusing synthetic events. Any ideas to do a quick fix? There must be a way to wrap action so that we call event.persist() before it runs?

@mallik-svmx
Copy link

I'm having the same issue. Any workaround will be greatly appreciated.

@shilman
Copy link
Member

shilman commented Nov 5, 2019

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.39 containing PR #8690 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Nov 5, 2019
@tay1orjones
Copy link
Contributor

This shouldn't be closed. The PR explicitly states

"The issue #6471 still persists, but at least we can avoid it by not passing react events to it directy by doing."

You might consider modifying your release script to require the use of specific keywords to auto-close issues, like GitHub's native functionality.

@petermikitsh
Copy link
Contributor

petermikitsh commented Aug 13, 2020

This is my solution to action logging concerning React synthetic events.

The key is to set view to undefined, because it's a reference to window, and logging window substantially slows down the performance of storybook (because window is giant-- that's by no means Storybook's fault or anything).

Use this facade for Storybook's action function throughout your project:

import { action } from '@storybook/addon-actions';

/* Partial event logging, as full logging can be expensive/slow
 * Invocation: partialLog('actionName')(eventObj, ...args)
 */
export const partialAction = (actionName) => {
  const beacon = action(actionName);
  return (eventObj, ...args) => {
    beacon({ ...eventObj, view: undefined }, ...args);
  };
};

Literal example in a story:

const Template: Story<AccordionProps> = (args) => <Accordion {...args} />;

export const Simple = Template.bind({});
Simple.args = {
  header: 'Hello World',
  body: 'This is my example accordion.',
  onChange: partialAction('onChange'), // fires whenever my accordion expands or collapses
};

This fix gets rid of React error message:

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're accessing the method `isDefaultPrevented` on a released/nullified synthetic event. This is a no-op function. If you must keep the original synthetic event around, use event.persist(). See https://fb.me/react-event-pooling for more information.

It's a good compromise of logging nearly all the data (except the 'view' property), and keeping Storybook fast.

@luissmg
Copy link

luissmg commented Feb 4, 2021

Hey @shilman, how are you? Do you know by any chance if this is being taken care of in the next release (6.2.0)? Since it is kinda related with performance

@shilman
Copy link
Member

shilman commented Feb 4, 2021

@luissmg nope! PRs welcome tho and I can try to squeeze it in

@luissmg
Copy link

luissmg commented Feb 4, 2021

Alright! Does the workaround on the top look "ok"? It might be something to be considered adding. I can try to handle that in the next couple days

@shilman
Copy link
Member

shilman commented Apr 1, 2021

We want to address this in 6.3. If you want to contribute to Storybook, we'll prioritize PR review for any fixes here. And if you'd like any help getting started, please jump into the #support channel on our Discord: https://discord.gg/storybook

@forivall
Copy link
Contributor

forivall commented Oct 28, 2021

Hey y'all, i noticed this issue, specifically, as it impacted performance since a structured clone of the window object is attempted on every action (as previously mentioned). I've drafted up a PR at #16514, and would like feedback on it! thanks!

@vdh
Copy link
Contributor Author

vdh commented Nov 10, 2021

So far, I've been using the workaround of manually bypassing events for every use of action (so, never inlining action calls), but it tends to create a lot of boilerplate:

import { action } from '@storybook/addon-actions';
import MyComponent, { MyComponentProps } from './MyComponent';

type OnChange = MyComponentProps['onChange'];
const changeAction = action('Change');
const mockOnChange: OnChange = event => {
  const newValue = event.target.value;
  changeAction(newValue);
};

@shilman
Copy link
Member

shilman commented Nov 12, 2021

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-beta.33 containing PR #16514 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Nov 12, 2021
@shilman
Copy link
Member

shilman commented Nov 12, 2021

¡Ay Caramba!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-rc.0 containing PR #16514 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

@michal-kurz
Copy link

The RC didn't help me with the synthetic event error, I'm still getting it in my project for actions auto-generated via react-docgen-typescript after upgrading (I checked I have 6.4.0-rc.6 in my package lock). This seems to be clogging the execution thread for multiple seconds with MuI Tooltip, as it autogenerates (and throws an error in) about 40 callbacks on hover. Unfortunately I wasn't able to reproduce this in a clean repo :/ Could someone please point me at a helpful direction to go from here?

@forivall
Copy link
Contributor

forivall commented Nov 23, 2021

@michal-kurz do you have any reproduction case that i can take a look at? (even if it's not clean). I'm not sure what exactly you mean by "clean repo", and i don't know which exact library you're talking about (material-ui? which one? etc).

Basically, i wrote this fix in a way to detect that the event is a synthetic event in a best-effort approach, which means that we can only detect that it's a synthetic event when using the development build of react and when function names arent mangled.

At the time, i didnt really look at what react is doing in creating a synthetic event or if view is a common property on native DOM events. Seems like it is a standard property on UIEvent, so i can revise my fix and make it more generic applying to all events, instead of specifically only making the change when the event is a react synthetic event.

@michal-kurz
Copy link

michal-kurz commented Nov 26, 2021

@forivall I did manage to make a reproduction scenario today! In my previous attempt I was on React version 17, but synthetic event pooling was removed in 17.0.0. I just remembered that today, went to my previous attempt, bumped down React version to 16 and sure enough the error is there, on 6.4.0-rc.6.

I made the scenario as a separate branch on a repo I made earlier to demonstrate a different issue. Here it is. The reproduction scenario is described in the branch README. I think the example will answer all of your questions better than I could :). But if there's something I can add, let me know!

Also, if you do find the time to look at it, would you mind answering the following question, please? Do you think the errors could be the reason for the extremely long delay before hovering the element and the tooltip actually showing up? Or is there something else you suspect? Finding out an answer to that would help me greatly :) Thank you very much!

@michal-kurz
Copy link

Any progress on this, @shilman, @forivall ? :)

@forivall
Copy link
Contributor

Sorry, i haven't had a chance to take a look yet.

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