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

Csf-tools: Add satisfies support to ConfigFile #20936

Merged

Conversation

zhyd1997
Copy link
Contributor

@zhyd1997 zhyd1997 commented Feb 5, 2023

Closes #20935

What I did

added satisfies support to .storybook/main.ts:

How to test

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

Comment on lines 142 to 147
if (t.isObjectExpression(decl) || t.isTSSatisfiesExpression(decl)) {
const expression = t.isTSSatisfiesExpression(decl)
? (decl.expression as Extract<t.Expression, t.ObjectExpression>)
: decl;
self._exportsObject = expression;
expression.properties.forEach((p: t.ObjectProperty) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AST comparison:
Screenshot 2023-02-05 at 18 35 03
Screenshot 2023-02-05 at 18 36 25

@zhyd1997 zhyd1997 changed the title feat: added satisfies to ConfigFile [csf-tools]. feat: added satisfies support to ConfigFile [csf-tools]. Feb 5, 2023
@shilman shilman changed the title feat: added satisfies support to ConfigFile [csf-tools]. Csf-tools: Fix satisfies support in ConfigFile Feb 5, 2023
@shilman shilman changed the title Csf-tools: Fix satisfies support in ConfigFile Csf-tools: Add satisfies support to ConfigFile Feb 5, 2023
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Nice one @zhyd1997 !! Thanks for this 🙏

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Feb 5, 2023

@shilman Thanks for your quick review!

I am not familiar with the codebase, so please review the PR carefully to make sure it will not break something, If you have any questions , please let me know.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

I didn't review closely enough. LMK what you think about this

decl.properties.forEach((p: t.ObjectProperty) => {
if (t.isObjectExpression(decl) || t.isTSSatisfiesExpression(decl)) {
const expression = t.isTSSatisfiesExpression(decl)
? (decl.expression as Extract<t.Expression, t.ObjectExpression>)
Copy link
Member

Choose a reason for hiding this comment

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

I'm suspicious of this line. I think it should be something like:

          let decl =
            t.isIdentifier(node.declaration) && t.isProgram(parent)
              ? _findVarInitialization(node.declaration.name, parent)
              : node.declaration;
          if(t.isAsExpression(decl) || t.isTSSatisfiesExpression(decl)) {
            decl = decl.expression;
          }
          if(t.isObjectExpression(decl)) {
            ... 
          } else {
            logger.warn(`Unexpected ${JSON.stringify(node)}`);
          }

The problem with the current code is that it assumes the satisfies expression is an ObjectExpression, which might not be the case. For example:

export default 'foo' satisfies Bar;

@stevezhu
Copy link

stevezhu commented Feb 5, 2023

Nice! Thanks for implementing this!

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.

Looks great to me, thanks @zhyd1997 !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants