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: Add experimental status API #22890

Merged
merged 7 commits into from
Jun 7, 2023
Merged

Feature: Add experimental status API #22890

merged 7 commits into from
Jun 7, 2023

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 2, 2023

This is a proposal/implementation of a new (manager) API for storybook.

This adds the ability to assign a status to stories. Multiple statusses to be exact.
This status information can then be used in the sidebar, or addons.

The API allows addons to call: api.setStatus({}).
You can see tests in this PR for more in-depth examples.

I am considering adding for information, it would be nice if there was also a displayname, perhaps and some additional information the addon could except for just the raw "statuc" (which is a enum in this proposal).

Feedback is welcome.

Use-cases for this API:

The a11y addon could use this addon to set which stories have passed the check, and which ones need attention.
The coverage addon can highlight which stories fall below the coverage threshold.

The plan is that long-term there will be a "run report/testrunner over all my stories" within storybook itself.
Setting a status will be a key way to store information about individual stories from addons, that then can be consumed everywhere.

@ndelangen ndelangen self-assigned this Jun 2, 2023
@ndelangen ndelangen requested a review from a team June 2, 2023 12:07
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 okay, I can imagine that we'd want more meta info about the status:

type StatusUpdate = {
  status: Status;
  reporter: string; // addon name, or similar
  summary?: string;
  description?: string;
};

setStatus({
  status: "error",
  reporter: "@storybook/addon-a11y",
  summary: "The story fails 2 accessibility tests",
  description: 'The following tests failed: "missing-label", "img-missing-alt"'
})

code/lib/manager-api/src/modules/stories.ts Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member Author

ndelangen commented Jun 2, 2023

@JReinhold you suggestion would have to look like this:

setStatus({
 'a-story-id': {
    'storybook/addon-a11y': {
      status: "error",
      reporter: "@storybook/addon-a11y",
      summary: "The story fails 2 accessibility tests",
      description: 'The following tests failed: "missing-label", "img-missing-alt"'
    } 
  }
})

The API needs to be able to set the status of multiple stories in 1 API-call to minimize re-renders.
And 1 storyId should be able to be linked to multiple statusses.

@tmeasday
Copy link
Member

tmeasday commented Jun 2, 2023

@ndelangen should you be able to set >1 different addon at once though? I was thinking more like:

setStatus(
  'storybook/addon-a11y', {
    'a-story-id': {
      status: "error",
      summary: "The story fails 2 accessibility tests",
      description: 'The following tests failed: "missing-label", "img-missing-alt"'
    }
})

@ndelangen
Copy link
Member Author

ndelangen commented Jun 2, 2023

When reading the data from store later, having the relationship be:

story_id => give me all statusses
story_id => give me status of addon_id

Is easier & faster when the story_id is the primary key.

I made it this structure with the sidebar needing this data later in mind.


I could -for sure- have the API be one way, and the actual data the other way, complicating the API code a little bit, but I'm more concerned about the confusion it would bring to addon-creators.

@JReinhold
Copy link
Contributor

I think @ndelangen's API suggestion makes the most sense. But given that the change is keyed by addon id, we don't necessarily need the reporter too.

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.

Nice job & nice tests!

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.

Reading the comments, I agree with the status object { status, reporter, description, details } (exact structure TBD) suggestion. I also think @tmeasday 's suggestion of setStatus('addon-id', statusObject) makes sense. We can store the data however we want, but that seems like a good API simplification

@tmeasday
Copy link
Member

tmeasday commented Jun 5, 2023

I could -for sure- have the API be one way, and the actual data the other way, complicating the API code a little bit, but I'm more concerned about the confusion it would bring to addon-creators.

I would have thought addon creators needing to know to set the right reporter field on every update (PS it's called context in the GitHub status API, FWIW) would be more prone to error. 🤷 But it's not a big deal.

@ndelangen
Copy link
Member Author

I'll make the change, thank you for the comments, all!

@ndelangen
Copy link
Member Author

ndelangen commented Jun 5, 2023

So here's the next iteration:

await expect(
API.updateStatus('a-addon-id', {
'a-story-id': {
status: 'pending',
title: 'an addon title',
description: 'an addon description',
},
'another-story-id': { status: 'success', title: 'a addon title', description: '' },
})
).resolves.not.toThrow();
expect(store.getState().status).toMatchInlineSnapshot(`
Object {
"a-story-id": Object {
"a-addon-id": Object {
"description": "an addon description",
"status": "pending",
"title": "an addon title",
},
},
"another-story-id": Object {
"a-addon-id": Object {
"description": "",
"status": "success",
"title": "a addon title",
},
},
}
`);

The reason for this change is:

The user of the API only has to state their addon_id once.
This add a limitation where 1 addon cannot set the status of multiple addons in 1 API-call.
Though 1 addon can still set the status of multiple stories.

The update object now has a bunch of presentational fields on it, like 'title' & 'description'. The enum value is now .status.
I've also added a custom data field, which can hold any data.

This has me thinking if this API is pretty much all we need for "reports".
@yannbf when you have some time to reflect on that together, let me know?

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.

LGTM apart from some small quality of life improvements.

code/lib/manager-api/src/modules/stories.ts Outdated Show resolved Hide resolved
@ndelangen ndelangen merged commit ced8bb7 into next Jun 7, 2023
17 checks passed
@ndelangen ndelangen deleted the norbert/status-api branch June 7, 2023 09:47
@JReinhold JReinhold changed the title Feature: add a status API Feature: Add experimental status API Jun 9, 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.

None yet

4 participants