-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(no-uninstalled-addons): add uninstalled plugin rule #96
feat(no-uninstalled-addons): add uninstalled plugin rule #96
Conversation
Solves #95 |
7c52709
to
9adae2f
Compare
9adae2f
to
5ec27e8
Compare
The only issue I still see here is that ESLint won't lint any file inside dot-folders by default and the user of this plugin will have to write something like this to lint the main.js file:
Not ideal but not sure if we can make this rule sort of override the default behaviour of ESLint. |
5ec27e8
to
1105b4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so nice @andrelas1 thank you so much for your contribution!!
I added a few comments if you don't mind
This is interesting. Maybe we do want to lint the entire storybook folder, so it allows us in the future to add more rules to other files like preview.js, manager.js etc |
47ea8e9
to
5a46547
Compare
5a46547
to
4f87d10
Compare
Co-authored-by: Andre Santos <andre.santos0906@gmail.com>
… found Co-authored-by: Andre Santos <andre.santos0906@gmail.com>
Co-authored-by: Andre Santos <andre.santos0906@gmail.com>
previously it was stripping out /preset or /register even if it was in the middle of the addo name such as @storybook/preset-create-react-app, which was incorrect. Co-authored-by: Andre Santos <andre.santos0906@gmail.com>
And add a section on how to configure if users have a custom config directory Co-authored-by: Andre Santos <andre.santos0906@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is AMAZING thank you so much @andrelas1 <3
🚀 PR was released in |
packageJson.dependencies = parsedFile.dependencies || {} | ||
packageJson.devDependencies = parsedFile.devDependencies || {} | ||
} catch (e) { | ||
throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relies too much on package.json being in the exact same folder, which is not always the case. For instance in our projects we tend to put the config files for storybook into its own folder inside config/storybook
while the project package.json
remains in the top level folder. Is there a chance to provide a configuration for this or extend the code so it drills up through the folders like npm looks like to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jdehaan thanks for this comment! Would something like this suffice?
{
"overrides": [
{
"files": ["whatever-is-your-config-dir/main.@(js|ts)"],
"rules": {
"storybook/no-uninstalled-addons": ["error", { "packageLocation": "./" }],
}
}
]
}
1 - You are able to set the location of the main.js file which will get the rule applied (currently possible)
2 - You are able to set the location of which the package.json that contains the addons is located at (needs to be implemented)
Another possible solution would be to use something like pkgUp
to find the closest package.json, which could work in your scenario, but still yield a false-positive result in scenarios where the package.json that was found is not the correct one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick feedback.
I also think that an explicit option is the safest way to go, these things won't jump up and down at all times and a one time configuration effort is fully ok and preferable to some black magic IMO. For very fancy configuration needs one could even provide an own configuration logic upfront generating pairs of main.js/package.json for bigger monorepos... So yes I would be fully happy with your configuration proposal!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works like a charm, beside that the new package features a smarter package.json resolution as I did not have to specify it anymore in many cases. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All thanks to @andrelas1 great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yannbf @andrelas1 Thank you!
Issue: #95
What Changed
Checklist
Check the ones applicable to your change:
yarn update-all
Change Type
Indicate the type of change your pull request is:
maintenance
documentation
patch
minor
major