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

Expose a way to passthrough after the request is found to be unhandled #238

Merged

Conversation

happycollision
Copy link
Contributor

@happycollision happycollision commented Nov 8, 2018

This PR aims to allow a request to be passed-through in the unhandledRequests hook.

Totally fine to say that you don't want this feature. I am adding it for me, but since it was simple, I figured add PR you here.

This allows you to actually pass certain unhandled requests through at runtime if you so choose. Sam Selikoff offered this as an example of a feature this unlocks (better than my original example). Currently, you can't really simply declare multiple multiple domains as a passthrough. You'd need something like:

this.get('api1.something.com', this.passthrough)
this.get('api2.something.com', this.passthrough)
// etc

This feature addition allows you to handle all of them with any runtime check you'd like to implement:

server.unhandledRequest = function(verb, path, request) {
  if (path.startsWith('aws')) {
    request.passthrough() // <-- This is what is now possible
  }
}
  • Add a .passthrough() method to the FakeRequest
  • Add tests.
  • Add documentation.

@happycollision happycollision force-pushed the allow-passthrough-after-unhandled branch from b38bf86 to 3b54218 Compare July 18, 2019 05:03
@happycollision
Copy link
Contributor Author

I created this PR a long time ago with the full intent to finish off that checklist above. However, I understand that new features like this may not be desirable in your project, and now that I am back to working on a project that depends on my fork, I'd love to finish this off if it is something that belongs in Pretender. I'd be happy to finish this PR (and also add proper TS typings for the new feature) if someone who maintains this project would let me know if the functionality is desired. I found it immensely helpful to be able to call .passthrough() on a request as illustrated in my example code above.

Any thoughts, @bantic, @mike-north, @trek, or @xg-wang?

@happycollision happycollision force-pushed the allow-passthrough-after-unhandled branch 2 times, most recently from 9984ed2 to fd6b942 Compare December 18, 2019 05:04
@happycollision
Copy link
Contributor Author

@xg-wang I went ahead and did this. Tests pass. I hope this feature is something you agree with. I have found it immensely useful.

@samselikoff
Copy link
Contributor

Being able to decide whether to passthrough a request at runtime is super useful & actually something we added to Mirage. I'm in favor!

README.md Outdated Show resolved Hide resolved
Copy link
Member

@xg-wang xg-wang left a comment

Choose a reason for hiding this comment

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

Yeah I think this feature is great, and your use case makes a lot of sense. Sorry for the delay!

test/passthrough_test.js Outdated Show resolved Hide resolved
@happycollision
Copy link
Contributor Author

I am happy to make the requested changes. I don’t blame you for not commenting sooner. My original example left a little something to be desired (like how is it useful?!).

Thanks for having a look. I’ll get around to those changes either tonight or within a few days.

Anytime `send` is called, the arguments are saved in the fake xhr
instance. They can subsequently be used in the `.passthrough()` method.
This allows us to pass through a request after it has been caught by
Pretender.
@happycollision
Copy link
Contributor Author

I should note that in addition to the adding a test, I altered the readme to more accurately reflect what happens when the user calls the new method.

@xg-wang xg-wang merged commit 455f422 into pretenderjs:master Dec 22, 2019
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.

3 participants