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

Make context-in-play-function aware of context variable name #120

Conversation

beaussan
Copy link
Contributor

@beaussan beaussan commented Feb 4, 2023

Issue: #118

What Changed

Previously, the context-in-play-function was forcing the context passthought to be named exactly context. This PR introduces changes to allow for any variable name to be used.

So now, given this, the linter will not fail

export const SecondStory = {
  play: async (ctx) => {
    await FirstStory.play(ctx)
  }
}

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

@yannbf
Copy link
Member

yannbf commented Feb 6, 2023

Hey @beaussan this is so awesome, thanks a lot for making this contribution!!
I only have one small worry, which is the following scenario:

export const SecondStory = Template.bind({})
SecondStory.play = async ({ canvasElement, ...ctx }) => {
    await FirstStory.play({ ...ctx }) // play function would fail, as canvasElement is not passed
}

I don't know if it's a good idea to allow that scenario to work, given that there's a high change that the subsequent play function will fail. I know it will only fail if it actually uses canvasElement, but this could open possibility for errors. WDYT?

@beaussan
Copy link
Contributor Author

beaussan commented Feb 6, 2023

Hey @yannbf , thanks !

It was already allowed previous to my change, but I agree this should error out too.

I could use the same system to extract all of the destructed args from the parent signature to make sure all are passed, WDYT ?

@yannbf
Copy link
Member

yannbf commented Feb 13, 2023

It was already allowed previous to my change, but I agree this should error out too.

Good point! I tried to reproduce the error in the current version of the plugin, and I believe it will be fine for JS projects but will error in Typescript, as canvasElement is needed as part of the type, so I think that's good enough? Unless you'd like to improve the PR, of course!

@beaussan
Copy link
Contributor Author

I think it should be good enough, typescript will caught it, and for most js users this will cover most cases.

Because given this :

export const SecondStory = {
  play: async ({ canvasElement, ...ctx }) => {
    const canvas = canvasElement;
    await FirstStory.play({ canvasElement: canvas, ...ctx })
  }
}

It shouldn't error out, but this will be really tricky to check and in my opinion, there could be higher case where a smarter rule fail on valide cases vs the current state of the lint rule

So I would say we can keep it this way and merge it, but I'll you decide @yannbf =)

@yannbf yannbf added the patch Increment the patch version when merged label Feb 21, 2023
@yannbf
Copy link
Member

yannbf commented Feb 21, 2023

Alright! I agree with you. This looks and behaves great! Thank you so so much for your contribution!

@yannbf yannbf merged commit 7e97a1f into storybookjs:main Feb 21, 2023
@github-actions
Copy link

🚀 PR was released in v0.6.11 🚀

@github-actions github-actions bot added the released This issue/pull request has been released. label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants