-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add support for custom id generation #69
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase now that I've merged the other PR?
README.md
Outdated
import FakeRest from 'fakerest'; | ||
import uuid from 'uuid'; | ||
|
||
const restServer = new FakeRest.Server('http://my.custom.domain', () => uuid.v5()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're in for a breaking change, we can afford changing the signature to a params object. Same for Fakerest.Server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nitpicks
aed08b2
to
f5e2506
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @fzaninotto
However I noticed a bug with some operations (such as DELETE) with the MSW implementation, which I'll need to fix before the release.
Hence this PR is still WIP.
@@ -205,7 +224,7 @@ export class BaseServer { | |||
|
|||
// handle collections | |||
const matches = request.url?.match( | |||
new RegExp(`^${this.baseUrl}\\/([^\\/?]+)(\\/(\\d+))?(\\?.*)?$`), | |||
new RegExp(`^${this.baseUrl}\\/([^\\/?]+)(\\/(\\w))?(\\?.*)?$`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this may break things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't noticed any and I don't see any other way to support custom ids
Co-authored-by: Francois Zaninotto <[email protected]>
Follow #68