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

🐛 ✨Fix eval setCurrentStory error + make logging better #837

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

nilshah98
Copy link
Contributor

@nilshah98 nilshah98 commented Oct 31, 2023

Description-

  • Fixed logging, earler we were just doing reject() without passing any error, hence it was passed as undefined and hence error was undefined and we were unable to parse message from it
  • Fixed issue where loading iframe.html as base preview URL would trigger the storyMissing event which would call reject and break flow. Solution for this was to open first component, instead of /iframe.html

@nilshah98 nilshah98 changed the title Fix eval set current story error 🐛 Fix eval setCurrentStory error + make logging better Oct 31, 2023
@nilshah98 nilshah98 changed the title 🐛 Fix eval setCurrentStory error + make logging better 🐛 ✨Fix eval setCurrentStory error + make logging better Oct 31, 2023
@nilshah98 nilshah98 marked this pull request as ready for review October 31, 2023 16:02
@nilshah98 nilshah98 requested a review from a team as a code owner October 31, 2023 16:02
src/utils.js Outdated
channel.on('storyMissing', reject);
channel.on('storyErrored', reject);
channel.on('storyThrewException', reject);
channel.on('storyMissing', () => reject(new Error('Story Missing')));
Copy link
Contributor

@ninadbstack ninadbstack Nov 1, 2023

Choose a reason for hiding this comment

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

Do we get original error passed to us, if yes we can wrap and rethrow it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even tried -

      channel.on('storyMissing', (err) => {
        reject(err);
      });

But this again resulted in -
[percy] TypeError: Cannot read properties of undefined (reading 'message')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, just learned something new about JS which explains this better.
Whenever a function name is passed as callback, it calls the function by whatever parameters are passed.
So, in this case

channel.on('storyMissing', reject)

This would call reject with whatever parameters are passed to eventListener, and since nothing is passed, hence we reject with undefined resulting in above error.

Copy link
Contributor

Choose a reason for hiding this comment

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

so basically story missing isnt passing anything ...
Also its not a new thing, its a standard thing that works across almost all languages ( except where brackets are not required for a function call, Ruby 👀 ), it works because its no different, it expects a function and you are passing a function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so basically story missing isnt passing anything ...

yes

Also its not a new thing, its a standard thing that works across almost all languages ( except where brackets are not required for a function call, Ruby 👀 ), it works because its no different, it expects a function and you are passing a function

Wasn't aware of this, thanks!

@@ -156,12 +156,12 @@ describe('percy storybook', () => {

await expectAsync(storybook(['http://localhost:8000']))
// message contains the client stack trace
.toBeRejectedWithError(/^Story Error\n.*\/iframe\.html.*$/s);
.toBeRejectedWithError(/^Story Errored\n.*\/iframe\.html.*$/s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Word change ? Is this same for v6 and v7

Copy link
Contributor Author

@nilshah98 nilshah98 Nov 1, 2023

Choose a reason for hiding this comment

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

This is a custom error that we are throwing.
Earlier in the test itself, we were mocking the error to be thrown, since storyErrored event was caught, but we reject the promise and passed nothing to it, so it returned undefined and fell back to the mocked error.

Okay, now I understand this a little better.
Earlier we were calling c (callback) with new Error('Story Error')

channel.on('storyErrored'. reject)

This would result in reject being called with the new Error('Story Error'), but since we have seen circumstances where nothing is passed to the callback which results in rejection with undefined, and hence added a custom message of Story Errored, instead of relying on default arguments.

Edit: Updated the mock to execute callback with no arguments, instead of new Error('Story Error')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored error throwing logic, so as to not update tests.

src/snapshots.js Show resolved Hide resolved
src/snapshots.js Show resolved Hide resolved
@nilshah98 nilshah98 force-pushed the fix-evalSetCurrentStory-error branch from 6b0129b to 6376d33 Compare November 2, 2023 18:42
Copy link
Contributor

@ninadbstack ninadbstack left a comment

Choose a reason for hiding this comment

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

LGTM

@nilshah98 nilshah98 merged commit 1fd15cb into master Nov 9, 2023
5 checks passed
@nilshah98 nilshah98 deleted the fix-evalSetCurrentStory-error branch November 9, 2023 05:58
@itsjwala itsjwala added the 🐛 bug Something isn't working label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants