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

experiment with universal channel #22939

Conversation

ndelangen
Copy link
Member

An experimental branch for a merged serverChannel into postMessageChannel

@ndelangen ndelangen requested a review from a team as a code owner June 6, 2023 11:17
@ndelangen ndelangen self-assigned this Jun 6, 2023
…cts to both.

leave serverChannel to point to main channel for backwards compatibility
@ndelangen ndelangen merged commit 9101c11 into norbert/server-channel-for-addons Jun 6, 2023
@ndelangen ndelangen deleted the norbert/experiment-merged-channels branch June 6, 2023 13:14
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.

Do we make it easy to disambiguate where a channel message comes from? IIRC it was always slightly painful to figure out which iframe a message came from during composition.

Or are you imagining that as the event names will be different for events coming from the server, you really won't need to disambiguate much?

const serverChannel = createWebSocketChannel({});
addons.setServerChannel(serverChannel);
window.__STORYBOOK_SERVER_CHANNEL__ = serverChannel;
window.__STORYBOOK_SERVER_CHANNEL__ = channel;
Copy link
Member

Choose a reason for hiding this comment

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

It feels weird that both channels come from '@storybook/channel-postmessage'. Should instead we make a new '@storybook/channel-duplex' or something?

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 perhaps in 8.0 we can simply have a @storybook/channel that can do any, with some config passed in, no need for separate packages?

I did not want to add another package.

Copy link
Member

Choose a reason for hiding this comment

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

Appreciate that. Maybe we should rename the package then (would that be bad too?) -- or at least indicate somewhere that 'channel-postmessage' is a bit of a misnomer as this now includes 2 channels.

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.

Do we make it easy to disambiguate where a channel message comes from? IIRC it was always slightly painful to figure out which iframe a message came from during composition.

Or are you imagining that as the event names will be different for events coming from the server, you really won't need to disambiguate much?

@ndelangen
Copy link
Member Author

Do we make it easy to disambiguate where a channel message comes from? IIRC it was always slightly painful to figure out which iframe a message came from during composition.

This is sort of true, yes. But this is because multiple previews can emit the same message types, and then the single manager needs to reply to the originator.

This problem is not really an issue where there is a 1-to-1 relationship.

Event-types are just strings really, so any connected party can listen for/emit anything they want.

Or are you imagining that as the event names will be different for events coming from the server, you really won't need to disambiguate much?

Any core message-types should be defined in @storybook/core-events IMHO.
That ensures everthing is aligned.. these message-types are just magic-strings after all...

There's always the possibility of event-types clashing.. This is why custom events should always have some unique prefix per addon. We do this for a11y-addon for example.


What's the use-case for knowing if an event came from the server?

@tmeasday
Copy link
Member

tmeasday commented Jun 7, 2023

I don't have any right now, so maybe it's fine :)

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.

2 participants