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

feat(no-uninstalled-addons): add uninstalled plugin rule #96

Merged
merged 6 commits into from
Jul 10, 2022

Conversation

andrelas1
Copy link
Contributor

@andrelas1 andrelas1 commented Jun 28, 2022

Issue: #95

What Changed

Checklist

Check the ones applicable to your change:

  • Ran yarn update-all
  • Tests are updated
  • Documentation is updated

Change Type

Indicate the type of change your pull request is:

  • maintenance
  • documentation
  • patch
  • minor
  • major

@andrelas1
Copy link
Contributor Author

Solves #95

@andrelas1 andrelas1 marked this pull request as draft June 28, 2022 17:16
@andrelas1 andrelas1 marked this pull request as ready for review June 30, 2022 17:01
@andrelas1 andrelas1 changed the title Draft: feat(no-uninstalled-addons): add uninstalled plugin rule feat(no-uninstalled-addons): add uninstalled plugin rule Jun 30, 2022
@andrelas1
Copy link
Contributor Author

andrelas1 commented Jun 30, 2022

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:

// .eslintignore file
# Allowlist 'test.js' in the '.build' folder
# But do not allow anything else in the '.build' folder to be linted
!.storybook

Not ideal but not sure if we can make this rule sort of override the default behaviour of ESLint.

tools/update-configs.ts Outdated Show resolved Hide resolved
Copy link
Member

@yannbf yannbf left a 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

@yannbf
Copy link
Member

yannbf commented Jul 1, 2022

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:

// .eslintignore file
# Allowlist 'test.js' in the '.build' folder
# But do not allow anything else in the '.build' folder to be linted
!.storybook

Not ideal but not sure if we can make this rule sort of override the default behaviour of ESLint.

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

@andrelas1 andrelas1 force-pushed the feat/no-uninstalled-addons branch 5 times, most recently from 47ea8e9 to 5a46547 Compare July 1, 2022 15:39
yannbf and others added 5 commits July 10, 2022 10:43
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>
Copy link
Member

@yannbf yannbf left a 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

@yannbf yannbf added the minor Increment the minor version when merged label Jul 10, 2022
@yannbf yannbf merged commit d1bb264 into storybookjs:main Jul 10, 2022
@github-actions
Copy link

🚀 PR was released in v0.6.0 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label Jul 10, 2022
packageJson.dependencies = parsedFile.dependencies || {}
packageJson.devDependencies = parsedFile.devDependencies || {}
} catch (e) {
throw new Error(
Copy link

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?

Copy link
Member

@yannbf yannbf Jul 11, 2022

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.

Copy link

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!

Choose a reason for hiding this comment

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

@yannbf Currently, if package.json is not found, the error is only logged to the console. It should probably be re-thrown or handled in a way that results in a non-zero exit status. At the moment, the linting process will silently pass.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @jdehaan @menelaos, a new version has been released including this feature! #102

And @menelaos that was intentional. We don't want people's projects to break so this plugin blocks them. The plugin now just acknowledges the issue and provides a link to the docs on how to fix it.

Thank you all!

Copy link

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!

Copy link
Member

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!

Copy link

Choose a reason for hiding this comment

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

@yannbf @andrelas1 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants