Throw appropriate errors for invalid instantiation. #113
Conversation
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
Thanks, this seems like a great enhancement.
src/ThemedStyleSheet.js
Outdated
|
|
||
| function invalidMethodError(invalidMethodName) { | ||
| return new TypeError(` | ||
| react-with-styles internal error: An invalid Style Interface has been registed. |
dmiller9911
force-pushed
the
issue-102-errors
branch
from
46ab866 to
1cec658
Compare
src/ThemedStyleSheet.js
Outdated
| } | ||
|
|
||
| function resolve(...styles) { | ||
| validateStyleInterface(); |
There was a problem hiding this comment.
create and resolve are pretty hot codepaths, so I'd like to avoid as much work as possible here. What do you think about wrapping all of these validate functions in something like
if (process.env.NODE_ENV !== 'production') {
...
}?
There was a problem hiding this comment.
I did some benchmarks of the change and didn't notice a difference between the typeof changes and the environment checks. If it matters, not sure on your environment support, but this would also assume/restrict the usage of this to a certain environment, without doing a null check on process and process.env. A babel plugin/webpack would probably simplify the check so it would definitely be better than not having it though.
What are your thoughts on an unimplemented function for the checks? It could get replaced on register, and we would no longer need to check each time create/resolve are called. This would add some complexity to readability though.
I would say the best two options in terms of performance are using the environment check you mentioned or the unimplemented function. What would you prefer?
There was a problem hiding this comment.
The environment checks can be compiled out by the app's build process, so it would be as if the code is deleted.
|
Thanks for the PR! |
Throw appropriate errors if a Style Interface has not been registered or an invalid Style Interface has been registered. Throw an appropriate error if a Theme has not been registered. Resolves: airbnb#102
dmiller9911
force-pushed
the
issue-102-errors
branch
from
1cec658 to
dc0011d
Compare
|
@dmiller9911 sorry I lost track of this PR. I'd still love to get this merged in, but it seems that some merge conflicts have popped up. Could you please rebase this onto latest master and let me know when it is ready again? |
3 participants
Throw appropriate errors if a Style Interface has not been registered or an invalid Style Interface has been registered.
Throw an appropriate error if a Theme has not been registered.
Resolves: #102