Skip to content

Conversation

underfin
Copy link
Member

…component rejected(#10027)

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
issues number: 10027

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

There is also the case where the timeout is exceeded which hasn't been handled. I'm not sure of what kind of error it should throw, maybe an new Error('timeout') but it would be nice to gather some feedback on this first

})

it('with error component', done => {
it('with error component + a prop named error', done => {
Copy link
Member

Choose a reason for hiding this comment

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

It's better to keep the old test and add a new one that shows the error

component: new Promise((resolve, reject) => {
setTimeout(() => {
reject()
reject('error')
Copy link
Member

Choose a reason for hiding this comment

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

Errors are usually new Error('message'), so better test it that way so it is closer to real scenarios

@underfin
Copy link
Member Author

Thank for your review. I did something for your suggestion.

: factory.resolved
}
}

Choose a reason for hiding this comment

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

I suggested you include a comment, preceded by the pull request number.

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

Successfully merging this pull request may close these issues.

3 participants