Skip to content
This repository was archived by the owner on May 3, 2023. It is now read-only.

Conversation

PChambino
Copy link
Contributor

Related to #8

src/index.js Outdated
}
} catch (error) {
onSetupError(error)
throw error
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should only throw the error if the user hasn't passed an onSetupError. If I'd passed an onSetupError I think I'd be surprised to see the error also being thrown by the library. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point. I added the throw so that during development something still gets logged to console since re-throwing the error shouldn't affect the application flow. Maybe using console.error would make the intention more clear. I will try console.error in development and push something to production to see if it behaves as expected.

@jackfranklin
Copy link
Contributor

Hey @PChambino this looks great! I have one thought about the error handling so let me know your thoughts, but very happy to merge this + publish once we've decided on that. Thank you :)

@PChambino
Copy link
Contributor Author

@jackfranklin 👋 Ok. I changed to console.error and come up with an extra thing. I added a context object to pass the flags to onSetupError since bad flags is one of the common errors of ElmComponent.init and pushing these flags together with the error to a bug tracking service should be helpful. (I didn't manage to push to production today, but will do first thing tomorrow.)

@PChambino
Copy link
Contributor Author

This has been running in production since this morning, everything working as expected. I am happy with this, let me know what you think.

@jackfranklin jackfranklin merged commit 17c5577 into thread:master May 10, 2019
@jackfranklin
Copy link
Contributor

@PChambino this is out as 1.0.0 now 👍

@PChambino PChambino deleted the connected-errors branch May 11, 2019 10:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants