Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

Dynamic fragments #189

Merged
merged 2 commits into from
Jun 19, 2017
Merged

Dynamic fragments #189

merged 2 commits into from
Jun 19, 2017

Conversation

tptee
Copy link
Contributor

@tptee tptee commented Jun 18, 2017

Fragments no longer rely on static routes for matching. This is the first and most important step in making consumer components code-splittable.

A few breaking changes fell out of this work:

  • Route matching in Fragment now depends on the order of the routes of its sibling Fragments, not the order of the static routes. Check out the test diffs for the behavioral differences. The biggest thing to remember is to put index routes (a nested /) last in the Fragment hierarchy.
  • Killed RouterProvider and provideRouter with 🔥 . We don't need them for matching hacks anymore, it's annoying boilerplate, and it rerenders the entire app on location change (wtf was I thinking when I wrote that???). We just connect() Fragment and Link now.
  • connect()ing our components resolves rendering bugs related to accessing mutating context. Redux and context don't play nicely AT ALL. We still use context to pass down parentRoute and parentId, but since there's rarely a need to dynamically change which route a Fragment matches on, we can safely assume that these values won't change after the first render (which is the prerequisite for using context safely with libraries like react-redux. We throw an error if forRoute changes for now. Supporting dynamic forRoute props would require a subscription system like that in react-redux, and ain't nobody got time for that.

@codecov-io
Copy link

codecov-io commented Jun 18, 2017

Codecov Report

Merging #189 into master will decrease coverage by 0.99%.
The diff coverage is 97.72%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #189   +/-   ##
=======================================
- Coverage   99.55%   98.56%   -1%     
=======================================
  Files          20       19    -1     
  Lines         227      209   -18     
=======================================
- Hits          226      206   -20     
- Misses          1        3    +2
Impacted Files Coverage Δ
src/enhancer.js 100% <ø> (ø) ⬆️
src/util/stringify-href.js 100% <ø> (ø) ⬆️
src/install.js 100% <ø> (ø) ⬆️
src/util/create-matcher.js 91.66% <100%> (-4.77%) ⬇️
src/components/link.js 100% <100%> (ø) ⬆️
src/components/fragment.js 94.59% <97.22%> (-5.41%) ⬇️

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 63e4cc1...bea6f45. Read the comment docs.

@tptee tptee merged commit 105563a into master Jun 19, 2017
@tptee tptee deleted the feature/dynamic-fragment branch June 19, 2017 04:15
@baebb
Copy link
Contributor

baebb commented Jun 19, 2017

🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥

};
}
constructor(props: Props, context: typeof Fragment.contextTypes) {
super(props, context);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe you need context to be declared in super() if it's not being used in the constructor

Copy link
Contributor

Choose a reason for hiding this comment

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

You also don't need to pass props, unless you're accessing this.props in the constructor. React will populate them both on the instance for you.

}
}

export default compose(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is some next-level composition 🔥

@kylecesmat
Copy link
Contributor

👋 RouterProvider 🔥 🔥

@faceyspacey
Copy link

@tptee you were thinking React Router does it. No harm no foul. Redux is king. Now we know. ...although there are a few things in react-redux that really should be revisited for even better perf. We'll see how Fiber affects react-redux.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants