fixup ts types and tweak createRouter #213
Conversation
package.json
Outdated
| "prop-types-extra": "^1.0.1", | ||
| "react-redux": "^5.0.7", | ||
| "react-redux": "^5.1.1", |
There was a problem hiding this comment.
fixes a bug with contextType
| render, | ||
| renderPending, | ||
| renderReady, | ||
| renderError, |
There was a problem hiding this comment.
practically i've never seen us save bytes here before. you almost always end up using createRender
| @@ -0,0 +1,94 @@ | |||
| // TypeScript Version: 3.0 | |||
|
|
|||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
stole this pattern from emotion
package.json
Outdated
| "lib", | ||
| "index.d.ts" | ||
| ], | ||
| "files": ["lib", "types/index.d.ts"], |
There was a problem hiding this comment.
did you run prettier on this? :p
| renderError, | ||
| }) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| render = |
There was a problem hiding this comment.
well, this would require updating the README at least
i'm not 100% sure about this. i put createRender at the top level because of bundle size concerns, but perhaps those are irrelevant
i think we would be better off exposing better higher-level abstractions, maybe – something like a createRelayRouter in found relay and a createHashRouter here, perhaps
how do you find yourself using createFarceRouter other than for the relay things with a custom resolver?
There was a problem hiding this comment.
in practice we've never used a lower level router without createRender, to do so you'd need to manage/implement ElementsRenderer yourself as well as StaticContainer. I don't think there are any people who need that level of bundle min/maxing esp if we never do it as the main consumers
There was a problem hiding this comment.
makes sense. update the docs then?
There was a problem hiding this comment.
the FarceRouter case could be handled via additional router factories but we're using createConnectedRouter to pass in an external redux store in a few places and that sort of auto drops you into the lowest level api.
There should also be a unified store enhancer i think
src/createFoundEnhancer.js
Outdated
| routeConfig, | ||
| matcherOptions, | ||
| }) { | ||
| return compose( |
src/utils/createFarceStore.js
Outdated
| ); | ||
|
|
||
| store.dispatch(FarceActions.init()); | ||
| store.startRouting(); |
There was a problem hiding this comment.
seemed like an easy way to save an import?
README.md
Outdated
| new Matcher(routeConfig, { matchStemRoutes: false }), | ||
| ), | ||
| ), | ||
| createFoundEnhancer({ |
There was a problem hiding this comment.
so, i wasn't totally a fan of this out of a sense of paranoia... my concern was something like – what if the user ended up with two versions of farce in the tree or something? double-importing action types is whatever, but the actual code... ?
|
err you may want to rebase this. sorry |
|
Huh, any idea why CI is failing? |
|
ok, needed to upgrade |
No description provided.