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

On client-side init error return rejected promise rather than throw #87

Merged
merged 1 commit into from
May 25, 2018

Conversation

damassi
Copy link
Contributor

@damassi damassi commented May 23, 2018

This swaps out an error with a rejected promise so that client UI doesn't break. Noticed when used in conjunction with @artsy/express-reloadable that when reloads occur and module ids increment an error occurs when really there's no problem.

(Really excellent lib -- was totally painless to setup compared to all of the others 👍)

@codecov
Copy link

codecov bot commented May 23, 2018

Codecov Report

Merging #87 into master will increase coverage by 2%.
The diff coverage is 83.33%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #87   +/-   ##
======================================
+ Coverage    88.5%   90.5%   +2%     
======================================
  Files          15      15           
  Lines         200     200           
  Branches       54      54           
======================================
+ Hits          177     181    +4     
+ Misses         21      17    -4     
  Partials        2       2
Impacted Files Coverage Δ
src/loadComponents.js 94.44% <83.33%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48303ba...bd0f38a. Read the comment docs.

@gregberge
Copy link
Owner

Thanks for this improvement, but if you use console.error, you have to add a return after.

@damassi
Copy link
Contributor Author

damassi commented May 23, 2018

Ah! Updated (was up working much too late).

@damassi damassi force-pushed the dont-throw branch 7 times, most recently from aef0ce6 to c351cff Compare May 23, 2018 19:05
@damassi
Copy link
Contributor Author

damassi commented May 23, 2018

I've updated tests to cover each error branch but I can't figure out how to stub window in jest in order to get to that first one flagged in codecov/patch.

}" is not found, client and server modules are not sync. You are probably not using the same resolver on server and client.`

console.error(errorMessage)
return Promise.reject(errorMessage)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should just do return Promise.reject(new Error(...)), it should be sufficient. So :

  • Please remove log
  • Please call Promise.reject with an 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.

return Promise.reject(new Error(...)) is much cleaner 👍

@gregberge
Copy link
Owner

@damassi don't worry about test coverage it is not a problem.

@damassi damassi force-pushed the dont-throw branch 2 times, most recently from 11ab531 to 0bc00fc Compare May 24, 2018 19:52
@damassi damassi changed the title Don't throw and break UI, but log error On client-side init error return rejected promise rather than throw May 24, 2018
@gregberge
Copy link
Owner

@damassi tests are broken :/

@damassi
Copy link
Contributor Author

damassi commented May 25, 2018

@neoziro - looks like a CI thing -- let me kick off another commit...

@gregberge gregberge merged commit d32c74c into gregberge:master May 25, 2018
@gregberge
Copy link
Owner

thanks!

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.

2 participants