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.

Sign up for GitHub

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

Manager: Add tags property to GroupEntry objects #29672

Merged
merged 7 commits into from
Nov 28, 2024
Merged

Manager: Add tags property to GroupEntry objects #29672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5f21921
refactor: Use better sanitisation for the intersect util
Sidnioulz Nov 20, 2024
103c6b2
Manager: Add tags property to GroupEntry objects
Sidnioulz Nov 20, 2024
247e65c
refactor: Further improve sanitisation in intersect
Sidnioulz Nov 20, 2024
2a2c8f2
Manager: Generalise tag intersection to root entries and ensure all e…
Sidnioulz Nov 26, 2024
1fd5f55
Merge branch 'next' into sidnioulz/group-entry-tags-intersection
shilman Nov 26, 2024
f0f2a4c
Fix check
shilman Nov 26, 2024
8496e3e
Merge branch 'next' into sidnioulz/group-entry-tags-intersection
shilman Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code/core/src/manager-api/lib/intersect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export default <T>(a: T[], b: T[]): T[] => {
// no point in intersecting if one of the input is ill-defined
if (!a || !b) {
if (!Array.isArray(a) || !Array.isArray(b)) {
Sidnioulz marked this conversation as resolved.
Show resolved Hide resolved
return [];
}

Expand Down
2 changes: 2 additions & 0 deletions code/core/src/manager-api/lib/stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ export const transformStoryIndexToStoriesHash = (
children: [childId],
}),
});
// same as the merge for the component conditional branch above.
acc[id].tags = intersect(acc[id]?.tags ?? item.tags, item.tags);
Sidnioulz marked this conversation as resolved.
Show resolved Hide resolved
}
});

Expand Down
69 changes: 69 additions & 0 deletions code/core/src/manager-api/tests/stories.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,75 @@ describe('stories API', () => {
tags: ['shared', 'two-specific'],
});
});

it('intersects story/docs tags to compute tags for group entries', () => {
const moduleArgs = createMockModuleArgs({});
const { api } = initStories(moduleArgs as unknown as ModuleArgs);
const { store } = moduleArgs;
api.setIndex({
v: 5,
entries: {
'a-sampleone': {
type: 'story',
id: 'a-sampleone',
title: 'A/SampleOne',
name: '1',
tags: ['shared', 'one-specific'],
importPath: './a.ts',
},
'a-sampletwo': {
type: 'story',
id: 'a-sampletwo',
title: 'A/SampleTwo',
name: '2',
tags: ['shared', 'two-specific'],
importPath: './a.ts',
},
'a-embedded-othertopic': {
type: 'docs',
id: 'a-embedded-othertopic',
title: 'A/Embedded/OtherTopic',
name: '3',
tags: ['shared', 'embedded-docs-specific', 'other'],
storiesImports: [],
importPath: './embedded/other.mdx',
},
'a-embedded-extras': {
type: 'docs',
id: 'a-embedded-extras',
title: 'A/Embedded/Extras',
name: '3',
tags: ['shared', 'embedded-docs-specific', 'extras'],
storiesImports: [],
importPath: './embedded/extras.mdx',
},
},
});
const { index } = store.getState();
// We need exact key ordering, even if in theory JS doesn't guarantee it
expect(Object.keys(index!)).toEqual([
'a',
'a-sampleone',
'a-sampletwo',
'a-embedded',
'a-embedded-othertopic',
'a-embedded-extras',
]);
// Acts as the root, so that the next level is a group we're testing.
expect(index!.a).toMatchObject({
type: 'root',
id: 'a',
children: ['a-sampleone', 'a-sampletwo', 'a-embedded'],
});
// The object of this test.
expect(index!['a-embedded']).toMatchObject({
type: 'group',
id: 'a-embedded',
parent: 'a',
name: 'Embedded',
tags: ['shared', 'embedded-docs-specific'],
});
});
// Stories can get out of order for a few reasons -- see reproductions on
// https://github.com/storybookjs/storybook/issues/5518
it('does the right thing for out of order stories', async () => {
Expand Down
1 change: 1 addition & 0 deletions code/core/src/types/modules/api-stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface API_GroupEntry extends API_BaseEntry {
type: 'group';
parent?: StoryId;
children: StoryId[];
tags: Tag[];
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about updating API_BaseEntry and propagating tags all the way up to the roots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would make the TS API a bit cleaner indeed. But I doubt it would make sense as there's virtually no way the intersection of all stories' tags makes sense to compute. It would likely be a waste of CPU resources to compute.

Copy link
Member

@shilman shilman Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I understand here, you are propagating N-2 levels up the tree. The roots add a single extra level, which means one extra intersection per root, which means almost no extra computation compared to this change. For the benefit of a cleaner, more consistent API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right that it doesn't cost that much and would simplify API for all users. Let me give it a go and see where it takes me!

I wonder if it would make sense to just document it as an empty array for 'root' entries, as that's what we expect would happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also realise that I do indeed go down N-2, which was not intended. I wanted tag intersections computed as an intersection of direct children for obvious performance reasons.

I'll rework the code to do all tag intersection in a separate pass, starting from leaves and going back up the tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up having to edit a few tests, because many test mock entries did not have tags defined in their input. This was a good opportunity to realise that tags, whilst mandatory on story entries, were never initialised and assumed to exist. I added an initialisation to an empty array when creating hash stories, prior to destructuring the index story item, to ensure types will remain valid in the future. Happy to remove that if you find it unnecessary.

}

export interface API_ComponentEntry extends API_BaseEntry {
Expand Down