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

[bugfix] Check for undefined before reading property in extractArgTypes.ts #18710

Merged
merged 3 commits into from
Aug 3, 2022
Merged

[bugfix] Check for undefined before reading property in extractArgTypes.ts #18710

merged 3 commits into from
Aug 3, 2022

Conversation

ProjektGopher
Copy link
Contributor

@ProjektGopher ProjektGopher commented Jul 14, 2022

Issue:
The type property on DocgenInfo is optional. By casting to any we lose the prompt that type could be undefined. So in some cases we're trying to read the property name of undefined.

// console error in browser
 Error loading story index:
 TypeError: Cannot read properties of undefined (reading 'name')
at Array.isEnum (extractArgTypes.js:40:1)
at verifyPropDef (extractArgTypes.js:68:1) at extractArgTypes.js:92:1
at Array.forEach (<anonymous>)
at extractArgTypes.js:87:1
at Array.forEach (<anonymous>)
at extractArgTypes (extractArgTypes.js:85:1) at enhanceArgTypes (enhanceArgTypes.js:8:1)
at prepareStory.js:111:1
at Array.reduce (<anonymous>)
// app/vue/src/client/docs/extractArgTypes.ts
function isEnum(propDef: PropDef, docgenInfo: DocgenInfo): false | PropDef {
  // cast as any, since "values" doesn't exist in DocgenInfo type
  const { type, values } = docgenInfo as any;
  const matched = Array.isArray(values) && values.length && type.name !== 'enum';
// lib/docs-tools/src/argTypes/docgen/types.ts
export interface DocgenInfo {
  type?: DocgenPropType;
  flowType?: DocgenFlowType;
  tsType?: DocgenTypeScriptType;
  required: boolean;
  description?: string;
  defaultValue?: DocgenPropDefaultValue;
}

What I did

// app/vue/src/client/docs/extractArgTypes.ts
function isEnum(propDef: PropDef, docgenInfo: DocgenInfo): false | PropDef {
  // cast as any, since "values" doesn't exist in DocgenInfo type
  const { type, values } = docgenInfo as any;
- const matched = Array.isArray(values) && values.length && type.name !== 'enum';
+ const matched = Array.isArray(values) && values.length && type && type.name !== 'enum';

@nx-cloud
Copy link

nx-cloud bot commented Jul 14, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dbef1ab. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ProjektGopher ProjektGopher changed the title check for undefined before reading property check for undefined before reading property in extractArgTypes.ts Jul 14, 2022
@ProjektGopher ProjektGopher changed the title check for undefined before reading property in extractArgTypes.ts [bugfix] Check for undefined before reading property in extractArgTypes.ts Jul 14, 2022
@webistomin
Copy link

Thanks! I run into the same problem😉

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.

Hey @ProjektGopher thank you so much for your contribution! 🚀

@yannbf
Copy link
Member

yannbf commented Jul 18, 2022

Hey @ndelangen could you check whether this change has to be done in other frameworks as well? Or at least Vue3? Thanks!

@yannbf yannbf added the bug label Jul 18, 2022
@ProjektGopher
Copy link
Contributor Author

ProjektGopher commented Jul 27, 2022

For anyone who needs to patch this before the PR gets merged, here's a useful workaround that'll get your CI builds passing as well:

Create patch.js in your project's .storybook directory

#!/usr/bin/env node
'use strict';

var fs = require('fs')
var path = require('path')

const patches = [
  { // app/vue/src/client/docs/extractArgTypes.ts (cjs)
    file: path.resolve(__dirname, '../node_modules/@storybook/vue/dist/cjs/client/docs/extractArgTypes.js'),
    find: "var matched = Array.isArray(values) && values.length && type.name !== 'enum';",
    replace: "var matched = Array.isArray(values) && values.length && type && type.name !== 'enum';"
  },
  { // app/vue/src/client/docs/extractArgTypes.ts (esm)
    file: path.resolve(__dirname, '../node_modules/@storybook/vue/dist/esm/client/docs/extractArgTypes.js'),
    find: "var matched = Array.isArray(values) && values.length && type.name !== 'enum';",
    replace: "var matched = Array.isArray(values) && values.length && type && type.name !== 'enum';"
  },
]

for (const patch of patches) {
  fs.readFile(patch.file, 'utf8', (err,data) => {
    if (err) {
      return console.log(err);
    }
    const result = data.replace(patch.find, patch.replace);
    fs.writeFile(patch.file, result, 'utf8', (err) => {
       if (err) return console.log(err);
    });
  });
}

Add this command to the scripts section of your package.json

"patch:storybook": "node .storybook/patch.js"

Add this command to your CI config right after installing dependencies

- run:
  name: Patch Storybook
  command: npm run patch:storybook

From now until patch, you can simply run npm run patch:storybook after (re)installing your node_modules.

@ndelangen
Copy link
Member

The code in question does not appear anywhere else. @yannbf

@ndelangen ndelangen merged commit c155a5c into storybookjs:next Aug 3, 2022
@ProjektGopher ProjektGopher deleted the fix/vue-client-extractArgTypes branch August 3, 2022 17:39
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Aug 5, 2022
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Aug 17, 2022
shilman pushed a commit that referenced this pull request Aug 17, 2022
…gTypes

[bugfix] Check for undefined before reading property in extractArgTypes.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch vue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants