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 commented Sep 27, 2016

need help with fix

@frenzzy frenzzy force-pushed the frenzzy:next-multiple-routes branch from cdaa1bc to 5d20d53 Sep 27, 2016
@langpavel

This comment has been minimized.

Copy link
Collaborator

langpavel commented Oct 6, 2016

@frenzzy Can you describe more?

@frenzzy

This comment has been minimized.

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

This comment has been minimized.

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 frenzzy:next-multiple-routes branch from 5d20d53 to 96e594b Oct 9, 2016
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 9, 2016

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
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.3%) to 88.235%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
frenzzy added a commit to frenzzy/universal-router that referenced this pull request Oct 9, 2016
koistya added 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
4 participants
You can’t perform that action at this time.