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

Matching by pattern with fallback #9

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

larry-x-yu
Copy link
Collaborator

@larry-x-yu larry-x-yu commented Jan 13, 2022

Summary of changes:

  1. Added support of fallback to base URL: when the query strings don't match, it'll automatically fallback to the one w/o query string if one has been configured, e.g.:
this.server.get('/api/graphql', handlerA); fetch('/api/graphql?foo=none');
would fallback to handerA if no match for '?foo=none' found.
  1. Added pattern matching: e.g.:
this.server.get('/api/graphql?foo=*&bar=*', handlerA); fetch('/api/graphql?foo=none&bar=123') would match handlerA.
  1. Added test cases to verify the new features

Testing done:
All test cases passed

Major concerns:

  1. Added a whole bunch code for supporting a simple pattern matching feature, is it overkill? - I used a Map for the matching as it helps in creating more meaning error messages down the road. Any suggestions is welcome!!!
  2. With the fallback support, do you foresee any backward compatibility issues? The biggest concern is breaking some of our customers' existing test cases.

@larry-x-yu larry-x-yu marked this pull request as ready for review January 13, 2022 20:01
README.md Outdated Show resolved Hide resolved
addon/pattern-matchers.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
addon/pattern-matchers.js Outdated Show resolved Hide resolved
addon/pattern-matchers.js Show resolved Hide resolved
addon/pattern-matchers.js Outdated Show resolved Hide resolved
addon/pattern-matchers.js Show resolved Hide resolved
tests/unit/pretender-query-param-handler-test.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@larry-x-yu larry-x-yu force-pushed the matching-by-pattern-with-fallback branch 2 times, most recently from 9b47487 to e92615f Compare January 14, 2022 19:57
Copy link
Collaborator

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

This looks great, thanks very much.

Two suggestions:

  1. I have a couple of inline suggestions for you to consider
  2. Can you please rebase the branch to have a clean commit history (probably this should be one commit, but either way I won't want to merge a commit history with an "Addressed review comment" commit as it makes git log and friends less useful.

Thanks very much!

README.md Outdated Show resolved Hide resolved
addon/pattern-matchers.js Show resolved Hide resolved
addon/pattern-matchers.js Outdated Show resolved Hide resolved
@larry-x-yu larry-x-yu force-pushed the matching-by-pattern-with-fallback branch from f8544f9 to b934ffd Compare January 14, 2022 21:39
@hjdivad hjdivad added the enhancement New feature or request label Jan 14, 2022
Copy link
Collaborator

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

Looks great, although i think you didn't intend to include 2b2f24f in the branch

@larry-x-yu larry-x-yu force-pushed the matching-by-pattern-with-fallback branch from b934ffd to d70b430 Compare January 14, 2022 23:07
@hjdivad hjdivad merged commit 31a0bc0 into rwjblue:main Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants