Core: Add context as a property of the context (self-referencing)

[kasperpeulen]

What I did

This PR uses the CSF changes here, they are best reviewed together:
https://github.com/ComponentDriven/csf/pull/98/files

This PR adds a self referencing context property to the story context:

storybook/code/lib/test/template/stories/before-each.stories.ts

Lines 60 to 65 in 054f661

	export const context_prop_is_available = {
	parameters: { chromatic: { disable: true } },
	async play({ context, canvasElement }) {
	await expect(context.canvasElement).toEqual(canvasElement);
	},
	};

This is useful for a lot of cases, but will be necessary when we implement the mount context field in a subsequent PR for 8.2 that always need to be destructured (see #27389):

storybook/docs/_snippets/my-component-play-function-composition.md

Lines 35 to 44 in b765c98

	export const CombinedStories: Story = {
	play: async ({ context, canvasElement }) => {
	const canvas = within(canvasElement);
	// Runs the FirstStory and Second story play function before running this story's play function
	await FirstStory.play(context);
	await SecondStory.play(context);
	await userEvent.type(canvas.getByTestId('another-element'), 'random value');
	},
	};

I consolidated the loader, before each, render and play contexts, so that it is the same for every phase. For example canvasElement and step are now always available:

storybook/code/lib/test/template/stories/before-each.stories.ts

Lines 67 to 79 in fd11b1a

	export const step_and_canvas_element_can_be_used_in_loaders_and_before_each = {
	parameters: { chromatic: { disable: true } },
	loaders({ step, canvasElement }) {
	step('loaders', async () => {
	await expect(canvasElement).toBeInTheDocument();
	});
	},
	beforeEach({ step, canvasElement }) {
	step('before each', async () => {
	await expect(canvasElement).toBeInTheDocument();
	});
	},
	};

The context is now always the same reference, which also allows people to mutate the context and use the mutation in a different phase. Similar as in vitest:

https://vitest.dev/guide/test-context#beforeeach-and-aftereach

We will need this behavior as well for the mount implementation. We will add a canvas context property in a @storybook/test loader. contex.canvas which will be equal to within(canvasElement) and will the default return type of the mount property (await mount() === context.canvas)

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for 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:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 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>

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ccabc98. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx run-many -t build -c production --parallel=3

---

Sent with 💌 from NxCloud.

[kasperpeulen]

Note that loaders, beforeEach, the step function, the renderer and the play function, all get the same context reference.

[kasperpeulen]

Changed this, as there are subtle differences between prepareMeta and prepareStory when the renderer has args, which were not relevant for those tests.

[kasperpeulen]

aborting as early as possible inside of applyloaders

[tmeasday]

How is this scenario possible? Is there an await in between the StoryRendering checking the abort signal and this line?

[kasperpeulen]

Yes the line below this line:
const loadResults = await Promise.all(loaders.map((loader) => loader(context)));
And notice that this is a loop, calling the project loaders first, and then the component and then the story loaders.

Whenever the user navigates to a new story, or force remounts, the StoryRender file will call:
this.abortController?.abort();

Now, the loaders can be cancelled half way, before it would only cancel after all loader are loaded.

[tmeasday]

Ahh OK, got it. Maybe a comment would be useful here (something like: "If an abort is received in between running each level of loaders, we want to avoid running the next level")

[kasperpeulen]

Okay, will add that to the mount PR!

[valentinpalkovic]

Very well done! I am not sure though, which kind of implications a self-referencing context object might have. The instrumenter is covered, which is the most obvious one. I am curious, whether there are cases where user's are serializing the context object. This would be a breaking change in this case.

[tmeasday]

I like this change!

[tmeasday]

I discussed this with someone recently (@shilman I think?). Alternatively we could get rid of composeConfigs and just pass ProjectAnnotations[] around -- and do the composing here.

That makes sense to me as it avoids the need for both WP + Vite to call composeConfigs at the right time.

It definitely doesn't makes sense to have both composeConfigs and normalizeProjectAnnotations.

Also I don't think these two lines belong in there:

storybook/code/lib/preview-api/src/modules/store/csf/normalizeProjectAnnotations.ts

Lines 43 to 48 in 8bb1298

	inferArgTypes,
	// inferControls technically should only run if the user is using the controls addon,
	// and so should be added by a preset there. However, as it seems some code relies on controls
	// annotations (in particular the angular implementation's `cleanArgsDecorator`), for backwards
	// compatibility reasons, we will leave this in the store until 7.0
	inferControls,

Instead, they should get added to the ProjectAnnotations[] somewhere.

[kasperpeulen]

Right, I like that. It seems better to do as little as possible JS in the webpack/vite template files.

Those 2 lines, should move to the addon-controls preview file right?

[tmeasday]

Those 2 lines, should move to the addon-controls preview file right?

Possibly, let's discuss with @shilman to make sure.

[tmeasday]

How is this scenario possible? Is there an await in between the StoryRendering checking the abort signal and this line?