Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store route on trie node #67

Merged
merged 1 commit into from Jun 9, 2019
Merged

Conversation

tornqvist
Copy link
Member

This reverts #66 in favor of storing the route on the trie node. Using a proxy callback worked for mitigating the immediate problem with reusing route callbacks but it also makes it difficult to register anything other than functions as route callbacks.

I.e. if I was to register a class as a route callback I'd probably want that class returned from .match and not a wrapper function. This is relevant if you'd want to register top level components as routes in choo.

This does however break the undocumented functionality of accessing the route on this in the callback. I dunno what that means for semver 馃

@yoshuawuyts
Copy link
Member

This does however break the undocumented functionality of accessing the route on this in the callback. I dunno what that means for semver thinking.

Ghmmm, yeah good one. We did make for an explicit patch to add this at some point - but I don't believe I ended up using it anywhere in Choo source.

I'm leaning towards ignoring it, and we'll hear it if it breaks. It's a quite small feature, and like you're saying: undocumented.

@goto-bus-stop
Copy link
Member

We can conservatively do a major for this and use it in Choo 7 @tornqvist

@tornqvist tornqvist merged commit 09035c9 into choojs:master Jun 9, 2019
@tornqvist tornqvist deleted the store-route-on-node branch June 9, 2019 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants