Add key to the element wrapped in the Route #40

ForsakenHarmony · 2018-01-15T15:56:31Z

Fixes #38

codecov · 2018-01-15T15:59:06Z

Codecov Report

Merging #40 into master will increase coverage by 3.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
+ Coverage   37.73%   41.07%   +3.33%     
==========================================
  Files           6        6              
  Lines          53       56       +3     
  Branches       12       14       +2     
==========================================
+ Hits           20       23       +3     
  Misses         24       24              
  Partials        9        9

Impacted Files	Coverage Δ
src/Route.js	`100% <100%> (ø)`	⬆️

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 7343784...4ea7821. Read the comment docs.

jorgebucaran · 2018-01-15T16:04:21Z

package.json

@@ -24,12 +24,15 @@
    "bundle": "rollup -i src/index.js -o dist/router.js -f umd -mn router -g hyperapp:hyperapp",
    "minify": "uglifyjs dist/router.js -o dist/router.js -mc pure_funcs=Object.defineProperty --source-map includeSources,url=router.js.map",
    "prepublish": "npm run build",
-    "format": "prettier --semi false --write 'src/**/*.js'",
+    "format": "prettier --semi false --write \"src/**/*.js\"",


@ForsakenHarmony Are you on windows? What's the purpose of this? Just curious.

jorgebucaran · 2018-01-15T16:06:27Z

src/Route.js

+    location: location
+  })
+
+  child.props.key = child.props.key || props.path


@ForsakenHarmony @zaceno Is this infallible?

Anyone familiar with ReactRouter, what's going on there?

I like this though.

props always gets added by h at least, if that's what you're asking

Nope, I mean does ReactRouter also add a key to the route component?

afaik, no, but they have component methods, which don't depend on elements that might be reused

@jorgebucaran is it infallible? ...well... this:

<main> <Route path="/show/secrets" render={SecretComponent} /> <AComponent /> <AnotherComponent /> <Route path="/show/secrets" render={AnotherSecretComponent} /> <ThirdComponent /> </main>

I e using the route as a switch to turn on or off multiple sibling components. Then the siblings will be keyed with the same key which of course is no good. ...unless the components have their own keys, in which case those keys will be used instead. So:

A - Before: By default oncreate won't get called for routed components. You have to manually key them to make it work. Beginners are required to understand the relationship between keys and lifecycle events. Higher threshold.

B - This proposal: You get automatic keys which make sense for the most part. And hence a lower threshold for beginners. But you run the risk of key-collisions and you don't understand why.

What about this?

var key = props.key || props.path + (props.render ? props.render.name + props.render.length : "")

Arrow functions have no name. 😞

They do if you assign them to a variable, but anonymously used, they don't, just ""

I guess we could use props.render.toString(), but that feels asking for trouble.

Keys have a pivotal role in the VDOM, and instead of hiding this them, we should ask users to properly key their routes instead.

Only child.props.key = child.props.key should be fine, but that too is subject to what we do with jorgebucaran/hyperapp#555.

/cc @frenzzy

jorgebucaran · 2018-01-15T16:07:26Z

package.json

    "release": "npm run build && npm test && git commit -am $npm_package_version && git tag $npm_package_version && git push && git push --tags && npm publish"
  },
  "babel": {
    "presets": "env"
  },
+  "jest": {
+    "testURL": "http://test/test"


Just leaving a comment here: https://facebook.github.io/jest/docs/en/configuration.html#testurl-string

Nice.

I wanted to do it in the test, but couldn't figure out how to do that

ForsakenHarmony · 2018-01-15T16:13:55Z

package.json

@@ -24,12 +24,15 @@
    "bundle": "rollup -i src/index.js -o dist/router.js -f umd -mn router -g hyperapp:hyperapp",
    "minify": "uglifyjs dist/router.js -o dist/router.js -mc pure_funcs=Object.defineProperty --source-map includeSources,url=router.js.map",
    "prepublish": "npm run build",
-    "format": "prettier --semi false --write 'src/**/*.js'",
+    "format": "prettier --semi false --write \"src/**/*.js\" \"test/**/*.js\"",


I used normal quotes, because single quotes seemed to break it on windows

lukejacksonn · 2018-01-15T16:31:46Z

Could you provide a description explaining what this PR does and what problem you are trying to solve?

ForsakenHarmony · 2018-01-15T16:33:57Z

#38

lukejacksonn · 2018-01-15T16:49:28Z

Thanks! So just to clarify the desired behaviour; in this case:

main([
  Route({ render: Nav }),
  Route({ path: '/users', render: () => div({ key: 'users' }, 'Some Users:')  }),
  Route({ path: '/users/:id', render: User }),
])

The first route gets no key
The second inherits its key users
The third route gets the key of /users/:id

Is this correct?

ForsakenHarmony · 2018-01-15T16:54:33Z

Yea, I guess so

lukejacksonn · 2018-01-15T20:08:27Z

The path seems like a sensible key to use. It is a good identifier for the element. The only doubt I have; is it guaranteed to be unique? Does it even matter if it is not? If it does matter, how should we document it? Otherwise SGTM 👍

ForsakenHarmony · 2018-01-16T01:34:40Z

it was mostly to address an issue I had with element reuse and oncreate not being called

would be the same thing with non unique paths ig

jorgebucaran · 2018-01-16T01:35:52Z

@lukejacksonn @ForsakenHarmony Zaceno explained it best here.

Add key to the element wrapped in the Route

1004077

jorgebucaran approved these changes Jan 15, 2018

View reviewed changes

jorgebucaran added the FEATURE label Jan 15, 2018

Move tests to a separate file

4ea7821

ForsakenHarmony commented Jan 15, 2018

View reviewed changes

jorgebucaran requested review from lukejacksonn and SkaterDad January 15, 2018 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add key to the element wrapped in the Route #40

Add key to the element wrapped in the Route #40

ForsakenHarmony commented Jan 15, 2018 •

edited

Loading

codecov bot commented Jan 15, 2018 •

edited

Loading

jorgebucaran Jan 15, 2018

jorgebucaran Jan 15, 2018

ForsakenHarmony Jan 15, 2018

jorgebucaran Jan 15, 2018

ForsakenHarmony Jan 15, 2018

zaceno Jan 15, 2018

jorgebucaran Jan 16, 2018 •

edited

Loading

jorgebucaran Jan 16, 2018

ForsakenHarmony Jan 16, 2018

jorgebucaran Jan 16, 2018

jorgebucaran Jan 16, 2018 •

edited

Loading

jorgebucaran Jan 15, 2018

ForsakenHarmony Jan 15, 2018

ForsakenHarmony Jan 15, 2018

lukejacksonn commented Jan 15, 2018

ForsakenHarmony commented Jan 15, 2018

lukejacksonn commented Jan 15, 2018

ForsakenHarmony commented Jan 15, 2018

lukejacksonn commented Jan 15, 2018

ForsakenHarmony commented Jan 16, 2018

jorgebucaran commented Jan 16, 2018

Add key to the element wrapped in the Route #40

Are you sure you want to change the base?

Add key to the element wrapped in the Route #40

Conversation

ForsakenHarmony commented Jan 15, 2018 • edited Loading

codecov bot commented Jan 15, 2018 • edited Loading

Codecov Report

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

jorgebucaran Jan 16, 2018 • edited Loading

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

jorgebucaran Jan 16, 2018 • edited Loading

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

lukejacksonn commented Jan 15, 2018

ForsakenHarmony commented Jan 15, 2018

lukejacksonn commented Jan 15, 2018

ForsakenHarmony commented Jan 15, 2018

lukejacksonn commented Jan 15, 2018

ForsakenHarmony commented Jan 16, 2018

jorgebucaran commented Jan 16, 2018

ForsakenHarmony commented Jan 15, 2018 •

edited

Loading

codecov bot commented Jan 15, 2018 •

edited

Loading

jorgebucaran Jan 16, 2018 •

edited

Loading

jorgebucaran Jan 16, 2018 •

edited

Loading