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

Support next() across multiple routes #49

Merged
merged 1 commit into from
Oct 9, 2016

Conversation

frenzzy
Copy link
Member

@frenzzy frenzzy commented Sep 27, 2016

need help with fix

@frenzzy frenzzy force-pushed the next-multiple-routes branch from cdaa1bc to 5d20d53 Compare September 27, 2016 20:17
@langpavel
Copy link
Collaborator

@frenzzy Can you describe more?

@frenzzy
Copy link
Member Author

frenzzy commented Oct 7, 2016

Sure, I found two problems:

  1. resolve() traverses all list of routes until one of them will return something, but next() function matches only single next route although it should traverses the remaining routes until one of them does not return something.
  2. Traversing routes must be strictly sequential and not parallel (similar to express.js). Bug: at the moment the router returns the result of the fastest asynchronous route instead of returning the result first of them.

@langpavel
Copy link
Collaborator

langpavel commented Oct 7, 2016

Ahhh, this looks like BIG issue, maybe bug..?

I don't like loop in RSK, it's potential source of DoS attack.. :-\

@frenzzy frenzzy force-pushed the next-multiple-routes branch from 5d20d53 to 96e594b Compare October 9, 2016 17:58
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 88.235% when pulling 96e594b on frenzzy:next-multiple-routes into 9740ad0 on kriasoft:master.

@koistya koistya merged commit a4be5ca into kriasoft:master Oct 9, 2016
frenzzy added a commit to frenzzy/universal-router that referenced this pull request Oct 9, 2016
koistya pushed a commit that referenced this pull request Oct 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants