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

Maintenance: Fix @ts-expect-error strict types #20981

Merged
merged 3 commits into from
Feb 17, 2023

Conversation

sheriffMoose
Copy link
Contributor

Closes #20877

What I did

  • fixed a few failing types while building sandboxes

How to test

  • CI Passes

const exportName = propKey(p);
if (exportName) {
let exportVal = p.value;
let exportVal = p.value as t.Expression;
Copy link
Member

@shilman shilman Feb 9, 2023

Choose a reason for hiding this comment

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

Why do we need all these extra type annotations? Is it causing a problem without?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, #20877

Copy link
Contributor Author

@sheriffMoose sheriffMoose Feb 9, 2023

Choose a reason for hiding this comment

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

@valentinpalkovic & I are affected, can't do changes without disabling strict mode and do few changes

@@ -109,7 +109,7 @@ export const getStorySortParameter = (previewCode: string) => {

if (t.isFunctionExpression(storySort)) {
const { code: sortCode } = generate.default(storySort, {});
const functionName = storySort.id.name;
const functionName = storySort.id?.name;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should add a TODO comment here to remind to take a look at this code.
if storySort.id can be undefined, it means that the code right after will generate invalid code, e.g.:

return undefined(a, b)

Copy link
Member

Choose a reason for hiding this comment

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

@sheriffMoose @ndelangen seems like this PR got merged but this was taken into account. Do you think this might bite us back if we don't at least add a TODO there?

Copy link
Member

Choose a reason for hiding this comment

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

If you feel like there should be a comment, and you have an idea what that comment should look like, sure.
I don't have a recollection of this context anymore.

Copy link
Member

Choose a reason for hiding this comment

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

The (now merged) code looks like this:

if (t.isFunctionExpression(storySort)) {
    const { code: sortCode } = generate.default(storySort, {});
    const functionName = storySort.id?.name;
    // Wrap the function within an arrow function, call it, and return
    const wrapper = `(a, b) => {
      ${sortCode};
      return ${functionName}(a, b)
    }`;
    // eslint-disable-next-line no-eval
    return (0, eval)(wrapper);
  }

so if storySort.id is optional, then storySort.id?.name could be undefined, right? meaning that functionName becomes undefined.

later on, return ${functionName}(a, b) will get executed. But then, if functionName is undefined, the evaluated code will be undefined(a,b), which will crash.

@yannbf yannbf added maintenance User-facing maintenance tasks and removed bug labels Feb 13, 2023
@yannbf yannbf changed the title chore(typescript): fix @ts-expect-error strict types Maintenance: Fix @ts-expect-error strict types Feb 13, 2023
@yannbf yannbf added the ci:daily Run the CI jobs that normally run in the daily job. label Feb 13, 2023
@valentinpalkovic
Copy link
Contributor

@sheriffMoose Can you merge the latest next into your branch? If the pipeline is green. Let's merge this. Please take care of Yanns small comment as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:daily Run the CI jobs that normally run in the daily job. maintenance User-facing maintenance tasks typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: [v7.0.0-beta.38/39] inexplicable error in csf-tools with angular sandbox
5 participants