Skip to content
This repository was archived by the owner on Apr 6, 2025. It is now read-only.

Add key to the element wrapped in the Route #40

Open
ForsakenHarmony wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add key to the element wrapped in the Route #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1004077
Add key to the element wrapped in the Route
ForsakenHarmony Jan 15, 2018
4ea7821
Move tests to a separate file
ForsakenHarmony Jan 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\"",
Copy link
Copy Markdown
Author

@ForsakenHarmony ForsakenHarmony Jan 15, 2018

Choose a reason for hiding this comment

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

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

"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"
Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 15, 2018

Choose a reason for hiding this comment

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

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

Nice.

Copy link
Copy Markdown
Author

@ForsakenHarmony ForsakenHarmony Jan 15, 2018

Choose a reason for hiding this comment

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

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

},
"devDependencies": {
"babel-preset-env": "^1.6.1",
"hyperapp": "0.18.0",
Expand Down
17 changes: 10 additions & 7 deletions src/Route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@ export function Route(props) {
exact: !props.parent
})

return (
match &&
props.render({
match: match,
location: location
})
)
if (!match) return

var child = props.render({
match: match,
location: location
})

child.props.key = child.props.key || props.path
Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 15, 2018

Choose a reason for hiding this comment

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

@ForsakenHarmony @zaceno Is this infallible?

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

I like this though.

Copy link
Copy Markdown
Author

@ForsakenHarmony ForsakenHarmony Jan 15, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 15, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

@ForsakenHarmony ForsakenHarmony Jan 15, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@zaceno zaceno Jan 15, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What about this?

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

Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 16, 2018

Choose a reason for hiding this comment

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

Arrow functions have no name. 😞

Copy link
Copy Markdown
Author

@ForsakenHarmony ForsakenHarmony Jan 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner

@jorgebucaran jorgebucaran Jan 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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


return child
}
15 changes: 15 additions & 0 deletions test/route.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { h } from "hyperapp"
import { Route } from "../src"

test("autokey", () => {
const route = h(Route, { path: "/test", render: () => h("div", {}, "test") })
expect(route.props.key).toBe("/test")
})

test("don't replace keys", () => {
const route = h(Route, {
path: "/test",
render: () => h("div", { key: "wow" }, "test")
})
expect(route.props.key).toBe("wow")
})