-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Controls: Fix reset button broken for !undefined URL values #18231
Controls: Fix reset button broken for !undefined URL values #18231
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 9583cea. 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 targetSent with 💌 from NxCloud. |
lib/preview-web/src/Preview.tsx
Outdated
@@ -252,7 +252,7 @@ export class Preview<TFramework extends AnyFramework> { | |||
const render = this.storyRenders.find((r) => r.id === storyId); | |||
const story = render?.story || (await this.storyStore.loadStory({ storyId })); | |||
|
|||
const argNamesToReset = argNames || Object.keys(this.storyStore.args.get(storyId)); | |||
const argNamesToReset = argNames || Object.keys(story.initialArgs); |
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.
@tmeasday could you take a look at this change? 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.
Hmm, this doesn't seem right.
This change ensures we always reset any arg that is initially set, but doesn't ensure we reset args that are set that weren't set initially.
I am thinking the reason this works is because if foo=!undefined
we end up unsetting foo
, in which case:
foo
(may have been) initially set, so is inObject.keys(story.initialArgs)
foo
is no longer set, so is not inObject.keys(this.storyStore.args.get(storyId)
Thus changing the code to check 1 but not 2 will reset foo
.
However, it introduces a new bug, as evidenced by the test case below, that if an arg is set but wasn't set in initialArgs
, it will never get reset.
Maybe we want something like [...new Set([...Object.keys(story.initialArgs), ...Object.keys(this.storyStore.args.get(storyId))])]
?
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.
Thank you for clarifying this! I will revisit this shortly. Thanks!
Thanks @Tomastomaslol ! I'll just wait for @tmeasday to give his opinion on that change, but overall functionality wise that's 👌 |
Thanks, @yannbf! 🙂 |
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 looking into this @Tomastomaslol!
lib/preview-web/src/Preview.tsx
Outdated
@@ -252,7 +252,7 @@ export class Preview<TFramework extends AnyFramework> { | |||
const render = this.storyRenders.find((r) => r.id === storyId); | |||
const story = render?.story || (await this.storyStore.loadStory({ storyId })); | |||
|
|||
const argNamesToReset = argNames || Object.keys(this.storyStore.args.get(storyId)); | |||
const argNamesToReset = argNames || Object.keys(story.initialArgs); |
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.
Hmm, this doesn't seem right.
This change ensures we always reset any arg that is initially set, but doesn't ensure we reset args that are set that weren't set initially.
I am thinking the reason this works is because if foo=!undefined
we end up unsetting foo
, in which case:
foo
(may have been) initially set, so is inObject.keys(story.initialArgs)
foo
is no longer set, so is not inObject.keys(this.storyStore.args.get(storyId)
Thus changing the code to check 1 but not 2 will reset foo
.
However, it introduces a new bug, as evidenced by the test case below, that if an arg is set but wasn't set in initialArgs
, it will never get reset.
Maybe we want something like [...new Set([...Object.keys(story.initialArgs), ...Object.keys(this.storyStore.args.get(storyId))])]
?
@@ -1133,7 +1133,7 @@ describe('PreviewWeb', () => { | |||
await waitForEvents([Events.STORY_ARGS_UPDATED]); | |||
expect(mockChannel.emit).toHaveBeenCalledWith(Events.STORY_ARGS_UPDATED, { | |||
storyId: 'component-one--a', | |||
args: { foo: 'a' }, | |||
args: { foo: 'a', new: 'value' }, |
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 seems like a bug? Shouldn't we be resetting new
to being unset?
Can you expand this test case (or create new test cases) to make sure we test:
- An arg with changed value (i.e.
foo
here) - An arg which was initially set, but is unset from the URL
- An arg that's set that wasn't in
initialArgs
(likenew
here).
Thanks!
@@ -1067,7 +1067,8 @@ describe('PreviewWeb', () => { | |||
|
|||
it('resets a single arg', async () => { | |||
document.location.search = '?id=component-one--a'; | |||
await createAndRenderPreview(); | |||
const preview = await createAndRenderPreview(); | |||
const onUpdateArgsSpy = jest.spyOn(preview, 'onUpdateArgs'); |
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.
I added expects for the call toonUpdateArgs
. To me, it made it easier to understand what was going inside of onResetArgs
. Please let me know if this is appropriate and what you think of the testing in general 🙂
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.
I guess I just check the STORY_ARGS_UPDATED
event as basically the same thing as checking onUpdateArgs
, as that is the only place it is emitted. Probably the spy is a little more coupled to the implementation than strictly necessary but I'm happy to leave it.
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.
Great! Love the tests :)
}); | ||
}); | ||
|
||
it('resets all args when one arg is not set initially', async () => { |
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.
Is this test essentially the same as resets all args
above?
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.
Yes, good point 🤦
Issue: #18143
What I did
onResetArgs
reset all args to remove any undefined valuesHow to test
Kapture.2022-05-16.at.07.48.48.mp4