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

Fix path params match issue #159

Merged
merged 1 commit into from
Aug 22, 2020
Merged

Fix path params match issue #159

merged 1 commit into from
Aug 22, 2020

Conversation

nooop3
Copy link
Contributor

@nooop3 nooop3 commented Aug 21, 2020

Precondition

  • static route /ab1
  • static route /ab2
  • static route /ac
  • parametric route /:params
  • request path /abcdef

Expect

// params
{ params: 'abcdef' }

Actual

// params
{ params: 'bcdef'}

Related issues

#145

Bench Results

> node bench.js

lookup static route x 44,313,098 ops/sec ±0.73% (584 runs sampled)
lookup dynamic route x 3,390,240 ops/sec ±0.97% (583 runs sampled)
lookup dynamic multi-parametric route x 1,832,331 ops/sec ±0.47% (578 runs sampled)
lookup dynamic multi-parametric route with regex x 1,309,854 ops/sec ±0.37% (586 runs sampled)
lookup long static route x 3,282,500 ops/sec ±0.72% (591 runs sampled)
lookup long dynamic route x 2,423,513 ops/sec ±0.49% (590 runs sampled)
lookup static versioned route x 8,511,010 ops/sec ±0.47% (590 runs sampled)
find static route x 44,480,616 ops/sec ±0.34% (590 runs sampled)
find dynamic route x 3,888,604 ops/sec ±1.54% (568 runs sampled)
find dynamic multi-parametric route x 2,370,472 ops/sec ±0.88% (585 runs sampled)
find dynamic multi-parametric route with regex x 1,819,445 ops/sec ±0.47% (591 runs sampled)
find long static route x 5,062,864 ops/sec ±0.33% (590 runs sampled)
find long dynamic route x 3,740,761 ops/sec ±0.33% (591 runs sampled)
find static versioned route x 10,959,874 ops/sec ±0.36% (591 runs sampled)

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina requested a review from delvedor August 21, 2020 13:56
Copy link
Owner

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@delvedor delvedor merged commit c82e801 into delvedor:master Aug 22, 2020
@delvedor
Copy link
Owner

Released in v3.0.4, thanks!

nooop3 added a commit to nooop3/find-my-way that referenced this pull request Aug 22, 2020
mcollina pushed a commit that referenced this pull request Aug 24, 2020
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.

3 participants