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

Core: Unified story specifiers #16220

Merged
merged 23 commits into from
Oct 7, 2021

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Oct 3, 2021

Issue: Specifiers were a bit of an unspecified (ha) mess.

Telescoping on #16160

What I did

  • Got a common type for specifiers working in all the places we use them

What I still need to do

  • Fix the tests
  • Ensure (at least in theory) it'll work in windows.

@tmeasday tmeasday mentioned this pull request Oct 3, 2021
10 tasks
@nx-cloud
Copy link

nx-cloud bot commented Oct 3, 2021

Nx Cloud Report

CI ran the following commands for commit 87008af. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

Comment on lines +188 to +191
STORIES: stories.map((specifier) => ({
...specifier,
importPathMatcher: specifier.importPathMatcher.source,
})),
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to add this little hack as the regexps weren't being serialized properly

@tmeasday tmeasday force-pushed the 16047-refactor-specifiers branch from 02ad3c6 to e0b29fd Compare October 5, 2021 06:08
tmeasday added a commit to storybookjs/addon-ie11 that referenced this pull request Oct 5, 2021
@tmeasday tmeasday marked this pull request as ready for review October 6, 2021 11:51
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.

Looking great esp the nice tests! 💯

Just one minor request 🙏

addons/storyshots/storyshots-core/src/api/index.ts Outdated Show resolved Hide resolved
lib/core-server/src/utils/stories-json.ts Outdated Show resolved Hide resolved
// Watchpack passes paths either with no leading './' - e.g. `src/Foo.stories.js`,
// or with a leading `../` (etc), e.g. `../src/Foo.stories.js`.
// We want to deal in importPaths relative to the working dir, or absolute paths.
const importPath = watchpackPath.startsWith('.') ? watchpackPath : `./${watchpackPath}`;
Copy link
Member

Choose a reason for hiding this comment

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

is this the only place we do this now? is this unified from before?

Copy link
Member Author

@tmeasday tmeasday Oct 7, 2021

Choose a reason for hiding this comment

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

Yes, any time we talk about paths we either use an importPath (JS-style relative to workingDir) [for things that end up inside the bundle] or an absolutePath [for things that don't]. The two things that liked to use "html-style" relative paths were:

  1. watchpack, which is why we do this here
  2. micromatch, which is why we have to futz with the generated regexp a little bit.

@shilman shilman added the core label Oct 7, 2021
@shilman shilman changed the title First pass at unified specifiers working Core: Unified story specifiers Oct 7, 2021
@shilman shilman added maintenance User-facing maintenance tasks and removed feature request labels Oct 7, 2021
@shilman shilman merged commit 4381ad2 into 16047-add-hmr-to-story-index-2 Oct 7, 2021
@shilman shilman deleted the 16047-refactor-specifiers branch October 7, 2021 05:07
@shilman shilman added this to the 6.4 PRs milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants