Skip to content
This repository has been archived by the owner on Apr 29, 2023. It is now read-only.

Add key to the element wrapped in the Route #40

Open
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 1 commit
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\"",
Copy link
Owner

Choose a reason for hiding this comment

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

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

"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
Owner

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
Author

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
Owner

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
Author

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
Owner

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
Author

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
Contributor

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
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
Owner

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
Author

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
Owner

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
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
}
10 changes: 10 additions & 0 deletions test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,13 @@ test("router", done => {

const unsubscribe = location.subscribe(main.location)
})

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

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