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: Integrate serverChannel into channel #22940

Merged
merged 23 commits into from
Jun 13, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 6, 2023

Closes #

Add listening capability to serverChannel on node's side
Fix data/args handling from serverChannel

What I did

  • addons can now extend the serverChannel from preset.ts
  • addons can now listen to channel messages and messages emitted from the serverChannel will appear
  • addons can now emit to the channel and messages will also be send to the server
  • Channel class can now receive multiple transports
  • add deprecation notices to serverChannel everywhere the channel should be used instead

How to test

add this to a manager.ts:

import * as React from 'react';
import { addons, types } from '@storybook/manager-api';

addons.register('ADDON_ID', (api) => {
  addons.add('ADDON_ID', {
    title: 'debugger',
    id: 'debugger',
    type: types.PANEL,
    match: ({ viewMode }) => viewMode === 'story',
    render: () => {
      const [state, setState] = React.useState('...');
      React.useEffect(() => {
        api.on('storybook/debug-b', (data) => {
          setState(JSON.stringify(data));
        });
      }, []);
      return React.createElement(
        'div',
        null,
        React.createElement('pre', null, state),
        React.createElement(
          'button',
          {
            type: 'button',
            onClick: () => {
              api.emit('storybook/debug-a', { foo: 'bar' });
            },
          },
          'button'
        )
      );
    },
  });
});

add this to preset.ts:

export const experimental_serverChannel = (channel: Channel) => {
  channel.on('storybook/debug-a', (event) => {
    console.log(event);
    channel.emit('storybook/debug-b', event);
  });
  return channel;
};
  1. opening the storybook and go to any (noon docs) story
  2. open the debugger panel
  3. click the button in the debugger panel
  4. notice how the CLI prints some data
  5. notice how the pre-tag gets filled with data.
  6. you can observe the websocket in the network tab

The channel should work correctly as well in build-mode

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

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

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

@ndelangen ndelangen requested a review from a team as a code owner June 6, 2023 13:23
@ndelangen ndelangen self-assigned this Jun 6, 2023
@ndelangen ndelangen requested a review from tmeasday June 6, 2023 13:23
@ndelangen ndelangen changed the title integrate serverChannel into channel, make serverChannel bi-directional and extendable Feature: integrate serverChannel into channel, make serverChannel bi-directional and extendable Jun 6, 2023
@tmeasday
Copy link
Member

tmeasday commented Jun 7, 2023

@ndelangen I made some comments on #22939

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.

Would like to consider creating a new kind of channel too, so postmessage channel doesn't implicitly contain a websocket channel (or just renaming postmessage channel). But how will this multiplexing work in RN where both channels are WS?

code/lib/core-server/src/dev-server.ts Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member Author

But how will this multiplexing work in RN where both channels are WS?

I should probably make the same multiplex change to the websocket channel package?
I hadn't really considered RN.

…eact-native (websocket, without duplicating the storybook-server-channel transport
@ndelangen ndelangen requested a review from tmeasday June 7, 2023 09:02
@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Jun 7, 2023

@ndelangen Will this PR get patched back to 7.0.x?

@ndelangen
Copy link
Member Author

@valentinpalkovic I wasn't planning to.

@ndelangen ndelangen requested a review from shilman June 7, 2023 16:07
@yannbf yannbf changed the title Feature: integrate serverChannel into channel, make serverChannel bi-directional and extendable Core: Integrate serverChannel into channel, make serverChannel bi-directional and extendable Jun 8, 2023
@yannbf
Copy link
Member

yannbf commented Jun 8, 2023

This looks amazing! I wonder if there's some documentation we can add to this?

@ndelangen
Copy link
Member Author

@yannbf the idea is that it's experimental. We'll try to do a few things internally, test if we like it and maybe iterate on it before announcing it to the public for general use.

Documenting it would certainly be welcome. How would you recommend we describe this change, in what document?

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.

LGTM! It's working great and fulfills the needs of the onboarding 👯

@tmeasday
Copy link
Member

tmeasday commented Jun 9, 2023

I should probably make the same multiplex change to the websocket channel package?
I hadn't really considered RN.

Maybe we should wait until we actually implement it in RN but I'm wondering if instead of the current implementation where @storybook/channel-X actually creates two channels (not even both of type X), we instead should add a new API to the @storybook/channel package where it can take two transports, and just do something like:

// in manager / preview code
import { websocketTransport } from '@storybook/channel-websocket';
import { postmessageTransport } from '@storybook/channel-postmessage';

this.channel = new Channel({ transports: [websocketTransport, postmessageTransport] });

@ndelangen
Copy link
Member Author

I really don't like the idea of creating yet-another package, that is counter to what I'm actually trying to achieve long term.

I don't agree that moving the code into each builder (twice because of SSv6 support) is the right move, but I'll do it.

I've also deprecated the createChannel function on channel-postmessage (but NOT on channel-websocket (in fact I completely reverted channel-websocket's code.
createChannel of postMessage is to be reverted as well I guess, nothing should be using it anymore anyway.

@shilman
Copy link
Member

shilman commented Jun 12, 2023

@tmeasday discussed with @ndelangen and he's going to revert the last set of changes and merge to unblock @yannbf . Let's spend a few minutes together tomorrow to discuss the ideal API here since the changes you suggested above result in a bunch of code duplication in the builders & @ndelangen doesn't want to introduce a new package. He has a better idea -- we think -- but would be good to hammer it out together synchronously.

@ndelangen ndelangen force-pushed the norbert/server-channel-for-addons branch from 6bdec4c to fd08096 Compare June 12, 2023 15:39
@ndelangen
Copy link
Member Author

@shilman I made a spike of moving the code to lib/channels, here: #23032

@ndelangen ndelangen merged commit b47b5f9 into next Jun 13, 2023
@ndelangen ndelangen deleted the norbert/server-channel-for-addons branch June 13, 2023 06:04
@shilman shilman changed the title Core: Integrate serverChannel into channel, make serverChannel bi-directional and extendable Core: Integrate serverChannel into channel Jun 13, 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.

5 participants