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 channelOptions for serverChannel #23615

Merged
merged 6 commits into from
Jul 27, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jul 26, 2023

Closes storybookjs/telejson#99

What I did

I noticed the error was caused by the serverChannel trying to parse functions and classes.

I'll continue to investigate why they are being sent.
It's orphaned by #9867
I can remove this for the serverChannel.

How to test

  • I cloned the repro submitted in the issue.
  • I reprodiced the issue.
  • I copy the dist from core-server after applying this fix.
  • I tried reproducing again, the problem went away.
  • I tested switching stories as well as HMR, both worked.

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>

@ndelangen ndelangen self-assigned this Jul 26, 2023
@ndelangen ndelangen added bug patch:yes Bugfix & documentation PR that need to be picked to main branch core ci:normal labels Jul 26, 2023
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

The change looks good, but can we add a test for this? Maybe in a follow-up PR if this is an urgent problem.

@ndelangen
Copy link
Member Author

ndelangen commented Jul 26, 2023

How would you imagine I test this @JReinhold ?

An unit-test seems kinda not-useful?, a e2e test is going to be a pain to setup (this is only really a thing in dev-mode).

@JReinhold
Copy link
Contributor

JReinhold commented Jul 26, 2023

My instinct was a unit test that mocks the socket, sends an event with a class/function that would have broken it, and see that what was actually sent is the correct thing. But I don't know enough about the context, maybe that is not useful or too complex compared to what it tests.

I can see how e2e or chromatic would be very hard in our current testing infrastructure.

@ndelangen
Copy link
Member Author

My instinct was a unit test that mocks the socket, sends an event with a class/function that would have broken it, and see that what was actually sent is the correct thing. But I don't know enough about the context, maybe that is not useful or too complex compared to what it tests.

I'd be happy to add such a test.
So what that's actually testing if the config is there, because telejson has unit-tests ensuring these options do what they are supposed to do.

@JReinhold
Copy link
Contributor

So what that's actually testing if the config is there, because telejson has unit-tests ensuring these options do what they are supposed to do.

We're testing that the channel doesn't break on classes/functions, regardless of which implementation it uses internally. So right now that would be ensuring that we keep setting those options and not remove them during a refactor. In 2 years time when for some reason we refactor to using another library than telejson, these tests would ensure that it still works.

@stevus
Copy link

stevus commented Jul 27, 2023

Thank you guys - really looking forward to this fix since it's causing Storybook to crash on my teams machines. If there's any testing I can help with I'm totally available.

@ndelangen ndelangen merged commit 46a96b3 into next Jul 27, 2023
@ndelangen ndelangen deleted the norbert/fix-server-channel-options branch July 27, 2023 15:33
This was referenced Jul 27, 2023
storybook-bot pushed a commit that referenced this pull request Aug 3, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
@github-actions github-actions bot mentioned this pull request Aug 3, 2023
32 tasks
storybook-bot pushed a commit that referenced this pull request Aug 4, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 7, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 7, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 7, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 7, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 8, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 9, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 9, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
storybook-bot pushed a commit that referenced this pull request Aug 9, 2023
…-options

Core: Fix channelOptions for serverChannel
(cherry picked from commit 46a96b3)
@kasperpeulen kasperpeulen removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Aug 9, 2023
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.

[Bug] name2.replace is not a function
4 participants