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

Feature: serverChannel API #22892

Closed
wants to merge 16 commits into from
Closed

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 2, 2023

Introduce a new API for storybook code:

An API to send messages to and from the serverChannel from addons.
Addons can connect to the serverChannel both in manager.ts and from preset.ts files, allowing an addon to do bi-directional communication.

This is very useful for addons that want to do some work in node, like let's say:

  • render something serverSide (serverside rendered languages)
  • replace some file (update some config in main.ts)
  • add a file (a story file perhaps (controls generating new stories)
  • this list goes on...

Changes:

  • Add an API for sending messages to the serverChannel from manager-api
  • Add listening capability to serverChannel on node's side
  • Fix data/args handling from serverChannel

Add listening capability to serverChannel on node's side
Fix data/args handling from serverChannel
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.

Looks good to me.
Small feedback on the API:

  1. Would it make sense to call it emit/onServer instead of emit/onServerAction, given that the existing is not called emitAction
  2. Should we have feature parity with the regular channel, so also have offServer, onceServer, getServerChannel?

code/lib/core-server/src/utils/get-server-channel.ts Outdated Show resolved Hide resolved
code/lib/manager-api/src/modules/channel.ts Outdated Show resolved Hide resolved
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Can we abstract it so that there is only one channel? This feels like yet another concept to have to educate addon authors about. Instead, addons could differentiate based on the event type.

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.

I agree with @shilman it would be best to abstract the two channels each context now has into a single "multiplexing" channel. But in the meantime, let's work to make the server channel as consistent as possible with the existing "manager<->preview" channel.

code/lib/manager-api/src/modules/channel.ts Outdated Show resolved Hide resolved
code/lib/core-server/src/utils/get-server-channel.ts Outdated Show resolved Hide resolved
@yannbf
Copy link
Member

yannbf commented Jun 5, 2023

I agree with Michael, if it's possible to get the same channel, then we get a nice API for addon authors, plus we get the already working mechanisms of on, off and once

@ndelangen ndelangen requested a review from a team as a code owner June 5, 2023 12:55
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.

This mostly LGTM. I'm a little unsure about the API naming, apart from that I agree with the approach so far.

@@ -8,8 +8,10 @@ import type { API, ModuleFn } from '../index';
export interface SubAPI {
getChannel: () => API_Provider<API>['channel'];
Copy link
Member

Choose a reason for hiding this comment

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

Should we have getServerChannel also? What about equivalents of off, once, etc?

I find the names onServerAction and emitServerAction kind of awkward. What about adding an extra argument to the other functions on('server', 'event', ...) or similar? Is that too confusing?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tmeasday I propose we do this, then later merge the 2 channels, and when we do that completely drop onServerAction and emitServerAction entirely.

So in the future .on will simply listen to both, and .emit will emit to both.

I don't want to do that today, because it would complicate/delay.

@@ -29,7 +29,7 @@ export async function storybookDevServer(options: Options) {
options.presets.apply<CoreConfig>('core'),
]);

const serverChannel = getServerChannel(server);
const serverChannel = await options.presets.apply('serverChannel', getServerChannel(server));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the "trick" to get (and possibly even modify/extend, the serverChannel: It's now a presetProperty.

This means addon creators can add a preset.js and add:

import { Channel } from '@storybook/channels';

export const serverChannel = async (channel: Channel) => {
  // do something with the channel
  return channel;
}

@ndelangen
Copy link
Member Author

Would it make sense to call it emit/onServer instead of emit/onServerAction, given that the existing is not called emitAction

Should we have feature parity with the regular channel, so also have offServer, onceServer, getServerChannel?

@JReinhold @shilman I just wanted to have something that's simple and work for now, minimal approach, prove it works.

Assuming we'll later work on merging the 2 channels, do we want to add 3 more API-methods?

I'm happy to make changes, do you feel particularly strong about this?

@ndelangen
Copy link
Member Author

ndelangen commented Jun 6, 2023

merging the 2 channels would involve:

  • making channel-postmessage also do inject a websocket transport
  • modify channel lib to support multiple transports
  • deal with edge cases where 1 transport is ready, the other is not (there's a buffer/flush system build-in)
  • deal with edge case where the serverChannel isn't available
  • update a bunch of unit-tests to cope with this change
  • assess risk, might this break anything?
  • edge cases of cyclical events
  • edge cases of events being received multiple times

It can be done, for sure. And I think we should do it eventually.
But for a few planned projects we just need bi-directional data-flow between node<>manager.

I decided to give it a try anyway:
#22939

@ndelangen
Copy link
Member Author

I'm going to close this and re-open a PR based on the same branch, so have the conversation make sense.

@ndelangen ndelangen closed this Jun 6, 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