-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Preview hooks: trigger effects after story render #7791
Conversation
This pull request is automatically deployed with Now. |
12a30c7
to
149ceba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions inline
@@ -226,8 +226,10 @@ export default function start(render, { decorateStory } = {}) { | |||
case 'story': | |||
default: { | |||
if (getDecorated) { | |||
render(renderContext); | |||
addons.getChannel().emit(Events.STORY_RENDERED, id); | |||
(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to catch here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be the handler? logger.error
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find a way to pass error callback to ReactDOM.render
which is the only source of asynchrony here.
Looks like they just use then
without catch
: https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOM.js#L393
So we won't actually catch anything. Hopefully, that means that React does the error handling for us
Issue: #7786 (comment) doesn't work because the effects are triggered too early, before the story is rendered
This also addresses #7407 (comment)