Router #244

fahad19 · 2017-07-28T13:49:08Z

NOTE: this PR should be rebase-merged to keep multiple contributors name in the history

Will be merged by @fahad19 once approved.

What's done

Introduced two new packages:

frint-router
frint-router-react

Documentation can be found in the diff.

coveralls · 2017-07-28T14:55:09Z

Coverage increased (+0.1%) to 97.519% when pulling 395a3de on router into 927667e on master.

coveralls · 2017-07-28T15:47:56Z

Coverage increased (+0.1%) to 97.519% when pulling d098ec7 on router into 927667e on master.

ghost

Amazing job guys 😍

ghost · 2017-07-31T08:06:25Z

packages/frint-router-react/src/Link.js

+      linkProps.className += ` ${activeClassName}`;
+    }
+
+    if (typeof type === 'undefined') {


what happened if type is equal to null ?

type prop is meant to contain type property for a button element. if somebody passes a type as a prop, then a <button> gets rendered, otherwise an <a>. I'm not even sure what should happen when somebody passes null there and what the developer expects in that case.

ghost · 2017-07-31T08:07:14Z

packages/frint-router-react/src/Link.spec.js

+    const linkContent = <span className="decorated">About</span>;
+
+    const wrapper = shallow(
+      <Link className="fancy-button" to="/about" type="button">{linkContent}</Link>,


fancy-button ❤️

ghost · 2017-07-31T08:08:48Z

packages/frint-router-react/src/Route.js

+    path: PropTypes.string,
+    exact: PropTypes.bool,
+    computedMatch: PropTypes.object,
+    component: PropTypes.oneOfType([PropTypes.node, PropTypes.func]),


Shouldn't we accept PropTypes.element as well here?

here we expect a definition of component, a class, not an instance (Component, not <Component />). that's why PropTypes.element doesn't fit here. so it's a function, because JavaScript :). also there can be stateless components, those are definitely functions.
actually PropTypes.node might not be needed. I am going to check.

ghost · 2017-07-31T08:10:37Z

packages/frint-router-react/src/Route.js

+
+      const RouteApp = nextProps.app;
+
+      this._appInstance = new RouteApp({


Couldn't we get the instance instead of creating a new object ?

we can't get because the instance does not live anywhere. these are App classes that are passed to <Route app={App} /> components.

since they are not registered to the Root app, like how we do it for Apps targeting Regions, they can't be obtained later from somewhere else. these Apps' lifecycle are contained by the <Route>.

ghost · 2017-07-31T08:11:27Z

packages/frint-router-react/src/Route.js

+    const matched = this.props.computedMatch || this.state.matched || null;
+
+    return ComponentToRender !== null && matched !== null
+      ? <ComponentToRender match={matched} />


just a matter of style but maybe a if here could be more readable

both look fine to me. I wouldn't change it unless many people find it unreadable.

I also think that both provide the same level of readability, therefore would keep it the way it is.

Yeah that's why I said a matter of style xD

ghost · 2017-07-31T08:13:13Z

packages/frint-router-react/src/Switch.js

+
+    React.Children.forEach(this.props.children, (element) => {
+      if (child !== null) {
+        return;


Shouldn't we explicitly return null here?

I don't think so. Here it's works as a continue statement for .forEach loop.

oh yeah I didn't see it sorry

ghost · 2017-07-31T08:15:38Z

packages/frint-router/src/matchFromHistory.js

+export default function matchFromHistory(pattern, history, options = {}) {
+  if (!pattern) {
+    return {
+      url: history.location.pathname,


Wouldn't it be better to check if history.location exists in the if?

I think history.location will always exist. At least it was always the case in unit tests with MemoryRouterService even before pushing any URLs in it. The idea here is to return any match if there's no pattern to match. @fahad19, am I correct?

ghost · 2017-07-31T08:16:12Z

packages/frint-router/src/matchFromHistory.js

+
+  const matched = re.exec(history.location.pathname);
+
+  if (!matched) {


I would check as well Array.isArray(matched)

markvincze · 2017-07-31T08:39:54Z

packages/frint-router-react/README.md

+    <div>
+      <Route path="/" component={HomePage} exact />
+      <Route path="/about" component={AboutPage} />
+    </div>


Just to make sure I understand: so this piece of code won't render any actual content, it's just for registering routes to components? Or will this actually display something?

it won't display until window url match the path
Really simplified version of this would be a function:

function Route({path, component}){ if (window.location.path === path) { return <component match={someDataAboutCurrentRouteLikeParams} /> } return null; }

it will render, once the current location in the browser matches to any of the paths. if there is a match, then the assigned component (HomePage or AboutPage) will render accodingly.

Ah ok, I think I understand, so the difference between this and using <Switch> is that this would simply render all the components which match, not just the first match, right?

exactly.

<Switch> would always render the first matching child, and stop there. If nothing else matches, then the last child (if it happens to have no path prop) will be rendered.

Much like switch in the language, with default as the last case.

markvincze · 2017-07-31T08:51:07Z

packages/frint-router-react/src/Link.js

+  handleClick = (e) => {
+    e.preventDefault();
+
+    // TODO avoid pushing route if it's current URL


How big is this task, can we do it now? 😺

@viacheslaff: could you kindly take a look at this?

I've added this TODO, it's the history library that issues a warning whenever the same route is pushed. there's a bit of discussion in here about whether it should warn or stay silent: remix-run/react-router#4467. but I guess <Link> shouldn't push the url if it's path exactly matches current url

markvincze · 2017-07-31T08:52:57Z

packages/frint-router-react/src/Link.js

+      linkProps.href = to;
+
+      return (
+        <a {...linkProps}>


wow, this is cool, haven't seen the spread being used like this before

markvincze · 2017-07-31T09:07:21Z

packages/frint-router/README.md

+1. `pattern` (`String`): Pattern to match against
+1. `options` (`Object`)
+  * `options.exact` (`Boolean`): Matches pattern exactly, defaults to `false`.
+  * `options.cache` (`Boolean`): Cache matches (TODO)


By TODO you mean this is not implemented yet? Shouldn't we just remove this from the readme?

a minimal amount of caching should be supported. repeated route matching can be costly on component re-renders. I will take care of it.

markvincze · 2017-07-31T09:17:31Z

site/content/guides/routing.md

+
+- `BrowserRouterService`: uses modern HTML5 History API
+- `HashRouterService`: for legacy browsers
+- `MemoryRouterService`: useful for tests


Which one should an average app developer use?
Isn't there a way to automatically figure out which one is the best choice with feature detection or something?

it's a bit difficult to decide which one should be used by default by anyone. if your application is backed by a server, then BrowserRouterService makes sense.

BrowserRouterService would keep URLs in your browser like /about/page/here.

But HashRouterService can work anywhere, as long as the target page is loaded, and URLs would look like this in the browser /index.html#/about/page/here.

And MemoryRouterService has nothing to do with the browser's API. it just imitates the behaviour of the other services and very useful in unit testing your app.

We want the developers to pick a service of their own needs when constructing the App, otherwise we end up having too much code in our bundles, that's never executed.

The developer needs to make a conscious decision about it. If you want feature detection, you have to do it yourself when returning the instance of the right service when your router provider is computed.

Might make sense to copy this over into the docs ;)

@jvantillo: good idea. I have copied the text, and will send a PR about how to choose the right router service in your app soon. but really want to release it first :D

also plan to revamp the homepage with more content to deliver a better message to developers. a brainstorm meeting invitation is coming soon.

markvincze

Looks awesome, a huge amount of work, and really nice, polished docs and sample! 👍

viacheslaff · 2017-07-31T09:04:26Z

packages/frint-react/package.json

-    "react-test-renderer": "^15.5.4"
+    "react-test-renderer": "^15.5.4",
+    "frint": "^2.0.1",
+    "frint-test-utils": "^2.0.1"


frint and frint-test-utils are duplicates. one pair should be removed

viacheslaff · 2017-07-31T09:06:39Z

packages/frint-react/package.json

@@ -38,7 +38,9 @@
    "prop-types": "^15.5.10",
    "react": "^15.5.4",
    "react-dom": "^15.5.4",
-    "react-test-renderer": "^15.5.4"
+    "react-test-renderer": "^15.5.4",


because enzyme, prop-types, react, react-dom , react-test-renderer are now in root package.json, they can be removed this package.json

viacheslaff · 2017-07-31T09:14:24Z

packages/frint-router-react/src/Link.js

+      linkProps.className += ` ${activeClassName}`;
+    }
+
+    if (typeof type === 'undefined') {


type prop is meant to contain type property for a button element. if somebody passes a type as a prop, then a <button> gets rendered, otherwise an <a>. I'm not even sure what should happen when somebody passes null there and what the developer expects in that case.

viacheslaff · 2017-07-31T09:24:32Z

packages/frint-router-react/src/Route.js

+    path: PropTypes.string,
+    exact: PropTypes.bool,
+    computedMatch: PropTypes.object,
+    component: PropTypes.oneOfType([PropTypes.node, PropTypes.func]),


here we expect a definition of component, a class, not an instance (Component, not <Component />). that's why PropTypes.element doesn't fit here. so it's a function, because JavaScript :). also there can be stateless components, those are definitely functions.
actually PropTypes.node might not be needed. I am going to check.

viacheslaff · 2017-07-31T09:56:04Z

packages/frint-router-react/src/Route.js

+    path: PropTypes.string,
+    exact: PropTypes.bool,
+    computedMatch: PropTypes.object,
+    component: PropTypes.func,


@JackTheRipper removed the PropTypes.node, because PropTypes.func is sufficient

coveralls · 2017-07-31T09:57:22Z

Coverage increased (+0.1%) to 97.519% when pulling e63722b on router into 927667e on master.

coveralls · 2017-07-31T15:57:06Z

Coverage increased (+0.1%) to 97.498% when pulling ffd0305 on router into 927667e on master.

* frint-router: caching. * frint-router: coverage. * frint-router: caching docs. * examples: require packages from local node_modules.

coveralls · 2017-08-01T08:17:38Z

Coverage increased (+0.1%) to 97.507% when pulling c76bbe1 on router into 927667e on master.

coveralls · 2017-08-01T08:29:40Z

Coverage increased (+0.1%) to 97.507% when pulling 9dab59c on router into 927667e on master.

fahad19 added minor PR labels Jul 28, 2017

fahad19 assigned fahad19, mAiNiNfEcTiOn and viacheslaff Jul 28, 2017

fahad19 requested review from reaktivo, a user, asci, mAiNiNfEcTiOn, alexmiranda, markvincze, ibsukru, discosultan, AlexDudar, nunorfpt, viacheslaff, iwwwi, jvantillo and gtzio July 28, 2017 13:49

fahad19 added 11 commits July 28, 2017 15:54

frint-router: base package.

9f81438

frint-router-react: base package.

c219096

examples: base for router app.

6f5181a

examples: top nav with links.

65a8815

examples: static routes (expected API usage).

a9f7cb0

frint-router: services.

9bfb102

frint-router: logic for matching paths.

67d61b1

frint-router: logic for streaming parsed named params.

dee12b0

examples: updated.

adb42d5

frint-router-react: initial commit for components.

79b127e

frint-router-react: path matching in components.

6c49ea3

examples: remove redundant Makefile.

395a3de

docs: list new packages in README.

d098ec7

ghost approved these changes Jul 31, 2017

View reviewed changes

markvincze reviewed Jul 31, 2017

View reviewed changes

markvincze approved these changes Jul 31, 2017

View reviewed changes

viacheslaff reviewed Jul 31, 2017

View reviewed changes

frint-router-react: Update propType for component prop in Route

e63722b

viacheslaff reviewed Jul 31, 2017

View reviewed changes

This was referenced Jul 31, 2017

frint-router: check result of RegExp.exec using Array.isArray #245

Merged

frint-router-react: Link: avoid pushing url if it's current one #246

Merged

viacheslaff added 2 commits July 31, 2017 17:51

frint-router: check result of RegExp.exec using Array.isArray (#245)

f06d916

frint-router-react: Link: avoid pushing url if its current one (#246)

ffd0305

mAiNiNfEcTiOn approved these changes Jul 31, 2017

View reviewed changes

discosultan approved these changes Jul 31, 2017

View reviewed changes

jvantillo approved these changes Aug 1, 2017

View reviewed changes

Router: caching (#247)

c76bbe1

* frint-router: caching. * frint-router: coverage. * frint-router: caching docs. * examples: require packages from local node_modules.

frint-react: cleaned up package.json

9dab59c

fahad19 merged commit 44f62b6 into master Aug 1, 2017

fahad19 deleted the router branch August 1, 2017 08:35

fahad19 mentioned this pull request Aug 1, 2017

frint-router: guide developers to choose the right service better #249

Open


		const RouteApp = nextProps.app;

		this._appInstance = new RouteApp({


		const matched = re.exec(history.location.pathname);

		if (!matched) {

Router #244

Router #244

Conversation

fahad19 commented Jul 28, 2017

NOTE: this PR should be rebase-merged to keep multiple contributors name in the history

What's done

coveralls commented Jul 28, 2017

coveralls commented Jul 28, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markvincze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 31, 2017

coveralls commented Jul 31, 2017

coveralls commented Aug 1, 2017

coveralls commented Aug 1, 2017