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

Add context.storyGlobals #95

Closed
wants to merge 2 commits into from
Closed

Conversation

tmeasday
Copy link
Contributor

@tmeasday tmeasday commented Jun 14, 2024

Following on from #92 -- add context.storyGlobals.

This is added to the very initial context (ContextForEnhancers) reflecting that we know it from back then.

Later (at render time) we'll also gain context.globals; which is the story's globals combined with the current value of the "user globals". Typically this is what user code would look at.

I'm not sure if we want to also add context.userGlobals to that layer (ContextForEnhancers) -- strictly speaking you cannot infer userGlobals from globals - storyGlobals. Then again it's unclear why you'd want to know the value of an overridden "user global".

Second question: should we make context.storyGlobals nullable, if the story does not set any global overrides?

📦 Published PR as canary version: 0.1.12--canary.95.1b1fb52.0

✨ Test out this PR locally via:

npm install @storybook/csf@0.1.12--canary.95.1b1fb52.0
# or 
yarn add @storybook/csf@0.1.12--canary.95.1b1fb52.0

tmeasday added a commit to storybookjs/storybook that referenced this pull request Jun 25, 2024
@ndelangen
Copy link
Member

Is this change backwards compatible?

@kasperpeulen
Copy link
Contributor

I thought we moved to a different model with initialGlobals and globals

@tmeasday
Copy link
Contributor Author

tmeasday commented Jul 17, 2024

@ndelangen

Is this change backwards compatible?

As much as adding anything to the context can be -- users can set arbitrary fields on the context, so theoretically it's possible someone out there is setting context.storyGlobals right now. I'd say it's pretty unlikely though.

@kasperpeulen

I thought we moved to a different model with initialGlobals and globals

globals is the name of the annotation (what you add to the CSF file) you use to set "story globals".

However, the actual calculated context.globals is a combination of context.storyGlobals (what's in the CSF) and the current value of userGlobals (the persisted globals, changed via UI) [not currently on context, see comment at the top of the ticket].

I'm pretty sure we shouldn't change the meaning of context.globals (a) because it'd be a (very) breaking change, and (b) I also think it'd be confusing.

You really only want access to context.storyGlobals to know if a global has been overridden by the current story. It's a specialized thing that typically you wouldn't look at.

@kasperpeulen
Copy link
Contributor

kasperpeulen commented Jul 17, 2024

You really only want access to context.storyGlobals to know if a global has been overridden by the current story. It's a specialized thing that typically you wouldn't look at.

But why do we need it then? Why would you want to know the global has been overridden? Except for manager concerns in storybook itself, which can just compare context.globals to the one that is persisted by storybook itself (userGlobals)?

My feeling is that context.globals is all the user needs in the context.

@tmeasday
Copy link
Contributor Author

We could consider not making available. Certainly addons need it in the manager to determine if a global has been overridden (theoretically you can check if userGlobals.X === globals.X but that's a bit tricky so we made it more explicit). At this stage it hasn't been needed in the preview yet.

One option would be to not put it in the context just yet (and just add it to the PreparedStory type) for now. I'm OK with that if that's your preference @kasperpeulen

@kasperpeulen
Copy link
Contributor

Sounds good to me @tmeasday !

@ndelangen ndelangen closed this Jul 19, 2024
ndelangen added a commit to storybookjs/storybook that referenced this pull request Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants