Throw an error if peer dependency is not installed #98
Conversation
| } | ||
|
|
||
| if (!Router) { | ||
| throw new Error('React Router is required as a peer dependency. Run `npm instal react-router`.') |
There was a problem hiding this comment.
@stevemao why are we throwing error if Router is not present, I thought the point was to make it optional?
There was a problem hiding this comment.
It is not? #97
@stevemao right now no. I guess we can move this https://github.com/paypal/react-engine/blob/master/lib/server.js#L29
Sorry if I misunderstood you.
There was a problem hiding this comment.
absolutely, but you might want to check if the user is intending to use react-router or not before throwing here.
once simple way is to examine if routes property exists in creatOptions createOptions.routes
There was a problem hiding this comment.
I see. Are you saying you want to make react-router optional if no router relevant options are passed?
My concern is if we make react-router optional, is there any other places we need to change?
There was a problem hiding this comment.
we need to do a similar check in client.js as well.
stevemao
force-pushed
the
peer-deps
branch
2 times, most recently
from
0c49dbc to
5bf5bf1
Compare
|
How does it look now? @samsel |
| @@ -38,6 +34,18 @@ var TEMPLATE = ['<script id="%s" type="application/javascript">var ', | |||
|
|
|||
| exports.create = function create(createOptions) { | |||
|
|
|||
| // safely require the peer-dependencies | |||
| var React = util.safeRequire('react'); | |||
| var Router = util.safeRequire('react-router'); | |||
There was a problem hiding this comment.
@stevemao if react-router is not installed, an error will be thrown on line 39 by default.
The correct way to do this would be to enclose this in a try/catch and conditionally throw.
condition === if (!Router && createOptions.routes)
stevemao
force-pushed
the
peer-deps
branch
2 times, most recently
from
375278a to
975d9ba
Compare
|
Updated. @samsel |
| } | ||
|
|
||
| if (!React) { | ||
| throw new Error('React is required as a peer dependency. Run `npm instal react` to install react.'); |
There was a problem hiding this comment.
thanks, but this is not needed right?
There was a problem hiding this comment.
Ok, I don't think we need to throw a new Error then. Just throw the caught error.
|
@samsel any news? |
Throw an error if peer dependency is not installed
No description provided.