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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

router: full on refactor #36

Closed
wants to merge 1 commit into from
Closed

router: full on refactor #36

wants to merge 1 commit into from

Conversation

yoshuawuyts
Copy link
Member

Whoop whoop, full on refactor! Been playing with this for idk how long. Mostly cleaned up internals so other routers can be built on top of this without having to worry about how to do composition (check out require('trie').mount() ✨). whoop!

Closes #32 #33 #34

Changed

  • removed multiplexed events
  • removed symbols dependency
  • removed internal method exposure; the need for that was an indication of
    lower level problems that this patch fixes

Added

  • matched routes can now return values (useful for streams)
  • Trie data structure can now be required doing require('wayfarer/trie')

Fixed

  • moved to using a single trie which greatly decreases lookup times
  • moved lookup logic to separate (internal) data structure which improves perf

Known issues

  • mounting multiple partial routes as direct children will error

@yoshuawuyts
Copy link
Member Author

Welp, no objections so far. I've seen some people ACK that they've seen it on twitter, so unless anyone has strong objections I'll merge it tomorrow morning (in like 8 hours) ✨

@yoshuawuyts
Copy link
Member Author

Just realized there's a bug in the resolution algorithm that's not being checked for; /foo and /:user currently cannot co-exist on the same router, therefor regular paths must be searched before named paths. This will require additional tests to be added / a tweak to the trie.search() function.

@yoshuawuyts
Copy link
Member Author

Annnd it's fixed! Publishing new version once tests pass.

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.

improve blank nested routes
1 participant