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

Core: Fix race-condition relating to addons.setConfig #23802

Merged
merged 3 commits into from Aug 21, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 11, 2023

What I did

When adding the API to set this from manager.ts I discovered the setConfig API seems broken due to a race condition between the manager UI rendering async and the manager being initiated before the manager.ts file is executed.

This was the order of operation:

  1. CONSTRUCT - manager-api context is contructed. The initial state is set.
  2. RENDER - the manager UI renders
  3. MANAGER - the manager.ts code is executed
  4. MOUNT - the manager UI mounts
  5. INIT - react call a hook to initiate all state/api modules in manager-api.

Due to this order + the fact that the event subscribers listening for the channel get executed upon the INIT phase, the fact the MANAGER phase emits and event, is missed.

My solution in this PR is not to change the order, but to change the location of the event-subscribers.
I moved them to the CONSTRUCT phase.

This should have no affects to use, but I had to change 2 things:

  • the unit tests that test the api/state modules
  • the api/state modules should use the provider given to them to listen/emit on the channel, because there's no fixed order in which the modules get created.

I hope this explains the changes in this PR enough.
I made and effort to cleanup some code, including the tests.

How to test

The changes are covered by unit tests.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

OK, makes sense to me! Thanks for cleaning stuff up a little.

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

I don't have a lot of context here but looks good to me overall!

@ndelangen ndelangen merged commit 13843a4 into next Aug 21, 2023
53 checks passed
@ndelangen ndelangen deleted the norbert/fix-setConfig-execution-order branch August 21, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants