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 advanced options to finetune glob #12055

Closed
wintercounter opened this issue Aug 16, 2020 · 10 comments
Closed

Add advanced options to finetune glob #12055

wintercounter opened this issue Aug 16, 2020 · 10 comments

Comments

@wintercounter
Copy link

wintercounter commented Aug 16, 2020

Is your feature request related to a problem? Please describe.
I have an issue with story loading in v6. The way changed how story files pattern is defined is causing a problem for us. Now it's only glob and only relative to main.js. We provide a global storybook instance to all our packages/projects (100+) so they can have all the same company standards, keep up to date easily, and they don't need to litter their machine with hundreds of Storybook installations.

Until now we used it like this:

const req = require.context('@', true, /\.?(story|stories|book)\.([jt]sx?|mdx)$/)
configure(req, module)

@ is resolved by Webpack to process.cwd() + 'src'. Seems like this is not possible in v6 now and on Windows it's an even bigger problem as glob doesn't suppose to understand Windows paths, so first I'd need to convert paths back and forth which is a nightmare, but anyway, it won't understand what is C: at the beginning either.

I also saw people having issues where they used complex filtering before, which is not possible now. Adding this would solve that problem also.

Another problem I saw is being able to pass options to minimatch. This could solve that also.

Describe the solution you'd like
I'd like to add advanced options to be able to configure globs, defined as an object.

{
    stories: [{
        basedir: path.resolve(process.cwd(), 'src'),
        glob: '**/*+(story|stories|book).+(ts|tsx|mdx|js|jsx)',
        filter: () => {}, // For advanced usecases
        globOptions: {} // options passed to minimatch
    }]
}

Are you able to assist bring the feature to reality?
Yes

@wintercounter
Copy link
Author

After some flexing, a user on Discord pointed me to the right direction which is replacing \ with / on Windows. This way I could define correct base directory in my glob.

However, I still think the request is valid to address other issues.

Also even tho everything seems to be work fine, i get the following warning now at startup:

WARN Unable to find main.js: C:\Work\Repos\my-pkg\C:\Work\Repos\my-storybook-provider-package\.storybook\main

Seems like if I have an absolute path in the glob, something will try to treat it as relative path to process.cwd(). WIth a GitHub search I found this warning in addon-docs, I guess it's coming from there.

@shilman
Copy link
Member

shilman commented Aug 16, 2020

@wintercounter i think there's a bug in the docs code. adding a PR to fix. #12057

@shilman
Copy link
Member

shilman commented Aug 17, 2020

Egads!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.12 containing PR #12057 that references this issue. Upgrade today to try it out!

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Aug 17, 2020
@shilman
Copy link
Member

shilman commented Aug 17, 2020

@wintercounter FYI #11181 (comment)

@wintercounter
Copy link
Author

wintercounter commented Aug 17, 2020

Thank you, the async option also awesome!

OFF TOPIC:
@shilman Just want to give a heads up about something minor I found. If I switch to main.js + stories from configure, the deprecated addDecorators API will stop work. Only works with the new export const decorators method. It's also true vice-versa. If I add decorators the new way then those decorators are not applied when I use configure. It was driving me mad for a few hours during migration :)

Sorry, I didn't want to open a new thread for this :D

@shilman
Copy link
Member

shilman commented Aug 17, 2020

yeah, the async thing is awesome 💯 . i didn't know about it until @ndelangen told me.

re: addDecorator, perhaps that's a regression? i don't think we intended to break that. cc @tmeasday

you were using addDecorator and not addDecorators, right? AFAIK there is no addDecorators.

@wintercounter
Copy link
Author

wintercounter commented Aug 17, 2020 via email

@tmeasday
Copy link
Member

Can we open an new issue for the decorator thing?, sounds like a bug for sure.

@shilman
Copy link
Member

shilman commented Aug 18, 2020

@tmeasday i think addDecorators was just a typo by @wintercounter, so no change necessary on our side. I could be wrong.

@wintercounter
Copy link
Author

No, I meant it's just a typo here :) But this was during migrating already working code from the old API.

Created new issue: #12100
I didn't create it before because I thought it's intentional, just not documented.

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

No branches or pull requests

3 participants