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
yannbf
merged 6 commits into
storybookjs:main
from
andrelas1:feat/no-uninstalled-addons
Jul 10, 2022
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
4f87d10
feat(no-uninstalled-addons): add uninstalled plugin rule
andrelas1 489c7d6
fix(no-uninstalled-addons): support /register and /preset on addon name
yannbf 168e0b0
fix(no-uninstalled-addons): handle scenario where package.json is not…
yannbf bebd0f6
feat(no-uninstalled-addons): support all kinds of addon declarations
yannbf 567e208
fix(no-uninstalled-addons): fix logic on addon names
yannbf f2bae3a
fix(no-uninstalled-addons): restrict rule to .storybook folder
yannbf File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| # no-uninstalled-addons | ||
|
|
||
| <!-- RULE-CATEGORIES:START --> | ||
|
|
||
| **Included in these configurations**: <ul><li>recommended</li></ul> | ||
|
|
||
| <!-- RULE-CATEGORIES:END --> | ||
|
|
||
| ## Rule Details | ||
|
|
||
| This rule checks if all addons in the storybook main.js file are properly listed in the root package.json of the npm project. | ||
|
|
||
| For instance, if the `@storybook/addon-links` is in the `.storybook/main.js` but is not listed in the `package.json` of the project, this rule will notify the user to add the addon to the package.json and install it. | ||
|
|
||
| As an important side note, this rule will check for the package.json in the same level of the .storybook folder. | ||
|
|
||
| Another very important side note: your ESLint config must allow the linting of the .storybook folder. By default, ESLint ignores all dot-files so this folder will be ignored. In order to allow this rule to lint the .storybook/main.js file, it's important to configure ESLint to lint this file. This can be achieved by writing something like: | ||
|
|
||
| ``` | ||
| // Inside your .eslintignore file | ||
| !.storybook | ||
| ``` | ||
|
|
||
| For more info, check this [ESLint documentation](https://eslint.org/docs/latest/user-guide/configuring/ignoring-code#:~:text=In%20addition%20to,contents%2C%20are%20ignored). | ||
|
|
||
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```js | ||
| // in .storybook/main.js | ||
| module.exports = { | ||
| addons: [ | ||
| '@storybook/addon-links', | ||
| '@storybook/addon-essentials', | ||
| '@storybook/addon-interactions', // <-- this addon is not listed in the package.json | ||
| ], | ||
| } | ||
|
|
||
| // package.json | ||
| { | ||
| "devDependencies": { | ||
| "@storybook/addon-links": "0.0.1", | ||
| "@storybook/addon-essentials": "0.0.1", | ||
| ' | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```js | ||
| // in .storybook/main.js | ||
| module.exports = { | ||
| addons: [ | ||
| '@storybook/addon-links', | ||
| '@storybook/addon-essentials', | ||
| '@storybook/addon-interactions', | ||
| ], | ||
| } | ||
|
|
||
| // package.json | ||
| { | ||
| "devDependencies": { | ||
| "@storybook/addon-links": "0.0.1", | ||
| "@storybook/addon-essentials": "0.0.1", | ||
| "@storybook/addon-interactions": "0.0.1" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Configure | ||
|
|
||
| Some Storybook folders use a different name for their config directory other than `.storybook`. This rule will not be applied there by default. If you want to have it, then you must add an override in your `.eslintrc.js` file, defining your config directory: | ||
|
|
||
| ```js | ||
| { | ||
| overrides: [ | ||
| { | ||
| files: ['your-config-dir/main.@(js|cjs|mjs|ts)'], | ||
| rules: { | ||
| 'storybook/no-uninstalled-addons': 'error', | ||
| }, | ||
| }, | ||
| ], | ||
| } | ||
| ``` | ||
|
|
||
| ## When Not To Use It | ||
|
|
||
| This rule is very handy to be used because if the user tries to start storybook but has forgotten to install the plugin, storybook will throw very weird errors that will give no clue to the user to what's going wrong. To prevent that, this rule should be always on. | ||
|
|
||
| ## Further Reading | ||
|
|
||
| Check the issue in GitHub: https://github.com/storybookjs/eslint-plugin-storybook/issues/95 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| /** | ||
| * @fileoverview This rule identifies storybook addons that are invalid because they are either not installed or contain a typo in their name. | ||
| * @author Andre "andrelas1" Santos | ||
| */ | ||
|
|
||
| import { readFileSync } from 'fs' | ||
| import { resolve } from 'path' | ||
|
|
||
| import { createStorybookRule } from '../utils/create-storybook-rule' | ||
| import { CategoryId } from '../utils/constants' | ||
| import { | ||
| isObjectExpression, | ||
| isProperty, | ||
| isIdentifier, | ||
| isArrayExpression, | ||
| isLiteral, | ||
| isVariableDeclarator, | ||
| isVariableDeclaration, | ||
| } from '../utils/ast' | ||
| import { Property, ArrayExpression } from '@typescript-eslint/types/dist/ast-spec' | ||
|
|
||
| //------------------------------------------------------------------------------ | ||
| // Rule Definition | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| export = createStorybookRule({ | ||
| name: 'no-uninstalled-addons', | ||
| defaultOptions: [], | ||
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: | ||
| 'This rule identifies storybook addons that are invalid because they are either not installed or contain a typo in their name.', | ||
| categories: [CategoryId.RECOMMENDED], | ||
| recommended: 'error', // or 'error' | ||
| }, | ||
| messages: { | ||
| addonIsNotInstalled: `The {{ addonName }} is not installed. Did you forget to install it?`, | ||
| }, | ||
|
|
||
| schema: [], // Add a schema if the rule has options. Otherwise remove this | ||
| }, | ||
|
|
||
| create(context) { | ||
| // variables should be defined here | ||
|
|
||
| //---------------------------------------------------------------------- | ||
| // Helpers | ||
| //---------------------------------------------------------------------- | ||
|
|
||
| // this will not only exclude the nullables but it will also exclude the type undefined from them, so that TS does not complain | ||
| function excludeNullable<T>(item: T | undefined): item is T { | ||
| return !!item | ||
| } | ||
|
|
||
| type MergeDepsWithDevDeps = (packageJson: Record<string, string>) => string[] | ||
| const mergeDepsWithDevDeps: MergeDepsWithDevDeps = (packageJson) => { | ||
| const deps = Object.keys(packageJson.dependencies || {}) | ||
| const devDeps = Object.keys(packageJson.devDependencies || {}) | ||
| return [...deps, ...devDeps] | ||
| } | ||
|
|
||
| type IsAddonInstalled = (addon: string, installedAddons: string[]) => boolean | ||
| const isAddonInstalled: IsAddonInstalled = (addon, installedAddons) => { | ||
| // cleanup /register or /preset from registered addon | ||
| const addonName = addon.replace(/\/register$/, '').replace(/\/preset$/, '') | ||
| return installedAddons.includes(addonName) | ||
| } | ||
|
|
||
| type AreThereAddonsNotInstalled = ( | ||
| addons: string[], | ||
| installedSbAddons: string[] | ||
| ) => false | { name: string }[] | ||
| const areThereAddonsNotInstalled: AreThereAddonsNotInstalled = (addons, installedSbAddons) => { | ||
| const result = addons | ||
| .filter((addon) => !isAddonInstalled(addon, installedSbAddons)) | ||
| .map((addon) => ({ name: addon })) | ||
| return result.length ? result : false | ||
| } | ||
|
|
||
| type GetPackageJson = (path: string) => Record<string, any> | ||
|
|
||
| const getPackageJson: GetPackageJson = (path) => { | ||
| const packageJson = { | ||
| devDependencies: {}, | ||
| dependencies: {}, | ||
| } | ||
| try { | ||
| const file = readFileSync(path, 'utf8') | ||
| const parsedFile = JSON.parse(file) | ||
| packageJson.dependencies = parsedFile.dependencies || {} | ||
| packageJson.devDependencies = parsedFile.devDependencies || {} | ||
| } catch (e) { | ||
| throw new Error( | ||
| 'Could not fetch package.json - it is probably not in the same directory as the .storybook folder' | ||
| ) | ||
| } | ||
|
|
||
| return packageJson | ||
| } | ||
|
|
||
| const extractAllAddonsFromTheStorybookConfig = ( | ||
| addonsExpression: ArrayExpression | undefined | ||
| ) => { | ||
| if (addonsExpression?.elements) { | ||
| // extract all nodes taht are a string inside the addons array | ||
| const nodesWithAddons = addonsExpression.elements | ||
| .map((elem) => (isLiteral(elem) ? { value: elem.value, node: elem } : undefined)) | ||
| .filter(excludeNullable) | ||
|
|
||
| const listOfAddonsInString = nodesWithAddons.map((elem) => elem.value) as string[] | ||
|
|
||
| // extract all nodes that are an object inside the addons array | ||
| const nodesWithAddonsInObj = addonsExpression.elements | ||
| .map((elem) => (isObjectExpression(elem) ? elem : { properties: [] })) | ||
| .map((elem) => { | ||
| const property: Property = elem.properties.find( | ||
| (prop) => isProperty(prop) && isIdentifier(prop.key) && prop.key.name === 'name' | ||
| ) as Property | ||
| return isLiteral(property?.value) | ||
| ? { value: property.value.value, node: property.value } | ||
| : undefined | ||
| }) | ||
| .filter(excludeNullable) | ||
|
|
||
| const listOfAddonsInObj = nodesWithAddonsInObj.map((elem) => elem.value) as string[] | ||
|
|
||
| const listOfAddons = [...listOfAddonsInString, ...listOfAddonsInObj] | ||
| const listOfAddonElements = [...nodesWithAddons, ...nodesWithAddonsInObj] | ||
| return { listOfAddons, listOfAddonElements } | ||
| } | ||
|
|
||
| return { listOfAddons: [], listOfAddonElements: [] } | ||
| } | ||
|
|
||
| function reportUninstalledAddons(addonsProp: ArrayExpression) { | ||
| // when this is running for .storybook/main.js, we get the path to the folder which contains the package.json of the | ||
| // project. This will be handy for monorepos that may be running ESLint in a node process in another folder. | ||
| const projectRoot = context.getPhysicalFilename | ||
| ? resolve(context.getPhysicalFilename(), '../../') | ||
| : './' | ||
| let packageJsonObject: Record<string, any> | ||
| try { | ||
| packageJsonObject = getPackageJson(`${projectRoot}/package.json`) | ||
| } catch (e) { | ||
| // if we cannot find the package.json, we cannot check if the addons are installed | ||
| console.error(e) | ||
| return | ||
| } | ||
|
|
||
| const depsAndDevDeps = mergeDepsWithDevDeps(packageJsonObject) | ||
|
|
||
| const { listOfAddons, listOfAddonElements } = | ||
| extractAllAddonsFromTheStorybookConfig(addonsProp) | ||
| const result = areThereAddonsNotInstalled(listOfAddons, depsAndDevDeps) | ||
|
|
||
| if (result) { | ||
| const elemsWithErrors = listOfAddonElements.filter( | ||
| (elem) => !!result.find((addon) => addon.name === elem.value) | ||
| ) | ||
| elemsWithErrors.forEach((elem) => { | ||
| context.report({ | ||
| node: elem.node, | ||
| messageId: 'addonIsNotInstalled', | ||
| data: { addonName: elem.value }, | ||
| }) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| //---------------------------------------------------------------------- | ||
| // Public | ||
| //---------------------------------------------------------------------- | ||
|
|
||
| return { | ||
| AssignmentExpression: function (node) { | ||
| if (isObjectExpression(node.right)) { | ||
| const addonsProp = node.right.properties.find( | ||
| (prop): prop is Property => | ||
| isProperty(prop) && isIdentifier(prop.key) && prop.key.name === 'addons' | ||
| ) | ||
|
|
||
| if (addonsProp && addonsProp.value && isArrayExpression(addonsProp.value)) { | ||
| reportUninstalledAddons(addonsProp.value) | ||
| } | ||
| } | ||
| }, | ||
| ExportDefaultDeclaration: function (node) { | ||
| if (isObjectExpression(node.declaration)) { | ||
| const addonsProp = node.declaration.properties.find( | ||
| (prop): prop is Property => | ||
| isProperty(prop) && isIdentifier(prop.key) && prop.key.name === 'addons' | ||
| ) | ||
|
|
||
| if (addonsProp && addonsProp.value && isArrayExpression(addonsProp.value)) { | ||
| reportUninstalledAddons(addonsProp.value) | ||
| } | ||
| } | ||
| }, | ||
| ExportNamedDeclaration: function (node) { | ||
| const addonsProp = | ||
| isVariableDeclaration(node.declaration) && | ||
| node.declaration.declarations.find( | ||
| (decl) => | ||
| isVariableDeclarator(decl) && isIdentifier(decl.id) && decl.id.name === 'addons' | ||
| ) | ||
|
|
||
| if (addonsProp && isArrayExpression(addonsProp.init)) { | ||
| reportUninstalledAddons(addonsProp.init) | ||
| } | ||
| }, | ||
| } | ||
| }, | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/storybookwhile the projectpackage.jsonremains 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
pkgUpto 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.
@yannbf Currently, if
package.jsonis 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.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 @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!
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!