Simplify adapter implementation

[geigerzaehler]

This is a proof of concept for a simplified Adapter class that should make it easier to implement existing and custom adapters. I’d like to get feedback on this proposal and continue working on it if it looks good.

Motivation

The motivation behind this is to make it easier to implement custom adapters. This is something I struggled with when trying to write an adapter that integrates with Cypress. The documentation on how to write custom adapters is sparse so I had to dig into the code.

1. The number of methods one needs to implement is quite large. (onConnect, onDisconnect, onIntercept, onPassthrough, onRecord, onReplay)
2. One needs to include code to persist the request in onRecord. If the implementer forgets this, recording will not work.
3. One needs to manually call pollyRequest.responds() in some hooks. Again, if this is omitted, the adapter will break.
4. Almost all request hooks call the same code at the end. (There are some exceptions but they just seem to be inconsistencies.)
5. The return value of the request hooks is returned by handleRequest(). This is not documented, hard to understand, and couples the request hook very tightly.

Proposal

The proposal addresses the five problems above by introducing a new SimpleAdapter class that can be used to implement adapters. This class requires the implementer to provide only three methods (onConnect, onDisconnect, and passthroughRequest). Using the SimpleAdapter allows for smaller and less complex adapter implementations.

SimpleAdapter extends the Adapter class. The original is retained for backwards compatibility.

Todo

• Flesh out docs for implementing custom adapter
• Implement puppeteer and node-http adapters with SimpleAdapter

[geigerzaehler]

The tests fail because of the ill-formatted commit message. The contributing guides does not mention this so I’ll need some help in making this pass.

[offirgolan]

@geigerzaehler thanks for taking the time to open this PR! I have a couple of questions/concerns:

1. Cypress already ships with the ability to mock & stub network requests, how would a Polly adapter fit in here?
2. Would drastically improving the documentation for creating an adapter be a good replacement for this?
3. The return value of the hooks is what gets returned to the end used. Meaning if they do fetch(), it will return a Response instance. If they do xhr.send() it will return undefined. In your current implementation, you're returning the pollyRequest instance always.

I ask the 2nd question because although some preexisting adapters may look similar, they have small differences that alter return values, how they respond to the original request, formatting responses before responding to the pollyRequest, etc.

If we were to implement a Simple Adapter, you would most likely need a passthroughRequest and respondToRequest methods as those are the most common needs among the 4 adapters.

[geigerzaehler]

Cypress already ships with the ability to mock & stub network requests, how would a Polly adapter fit in here?

There are three main reasons for using Polly. It has more features (request recording and replay), better ergonomics when defining fake servers, and it is modular and extensible. (We are not committed to cypress, so we want to be flexible to switch to something else)

Would drastically improving the documentation for creating an adapter be a good replacement for this?

Even with better documentation I feel that it would still be harder to implement an adapter than with this proposal. For example you would need to document that the implementer is required to call this.persister.recordRequest() instead of making the code do that for the implementer.

I ask the 2nd question because although some preexisting adapters may look similar, they have small differences that alter return values, how they respond to the original request, formatting responses before responding to the pollyRequest, etc.
If we were to implement a Simple Adapter, you would most likely need a passthroughRequest and respondToRequest methods as those are the most common needs among the 4 adapters.

I think my proposal addresses this issue. Concrete implementations will extend from SimpleAdapter but are required to implement passthroughRequest.

The return value of the hooks is what gets returned to the end used. Meaning if they do fetch(), it will return a Response instance. If they do xhr.send() it will return undefined. In your current implementation, you're returning the pollyRequest instance always.

Yes, the hooks always return a Polly request. This is so that this.handleRequest() always returns the Polly request. However, xhr.send() will not return the Polly request. I adjusted the implementation of the onConnect method of the XHR adapter to handle this properly.

I hope this answers all you questions and your concerns. I’m happy to answer more of them. In the meantime I’ll extend this PR with implementations of the puppeteer and node adapters to show that this approach also works well with them.

[offirgolan]

Ah, ok I see where you are going with this. I think the implementation is starting to look really good but I have some suggestions.

1. I'm not sure I understand the usage of flushResponse. If it's used to respond to the PollyRequest instance, then you've changed the timing of it by moving it to onRequestFinished. The PollyRequest should already have a response before the original request has been responded to since there are events and hooks that run in pollyRequest.respond that could modify the end response.
2. Instead of having a SimpleAdapter, we can just integrate this "sugar" into the default adapter since we're just adding some logic on top of the existing hooks that can be overridden.

[geigerzaehler]

I'm not sure I understand the usage of flushResponse. If it's used to respond to the PollyRequest instance, then you've changed the timing of it by moving it to onRequestFinished. The PollyRequest should already have a response before the original request has been responded to since there are events and hooks that run in pollyRequest.respond that could modify the end response.

The purpose of flushResponse is to ensure that a response has been delivered to the user calling the adapter before polly.flush() resolves. For example for the node adapter flushResponse() sends the response data to the request stream and returns only if the data has been sent successfully. This is to satisfy this test. flushResponse does not respond to the Polly request. This does still happen earlier in the onRecord, etc. hooks.

I have added some documentation for the flushResponse method.

Instead of having a SimpleAdapter, we can just integrate this "sugar" into the default adapter since we're just adding some logic on top of the existing hooks that can be overridden

That makes a lot of sense. I have changed that

[offirgolan]

The purpose of flushResponse is to ensure that a response has been delivered to the user calling the adapter before polly.flush() resolves.

What I meant was that the hook name is very confusing. What if we use the original hook?

async onRequestFinished() {
  // Do some custom logic
  fakeXhr.respond(...);

  return super.onRequestFinished(...arguments);
}

Either that or give the hook a more descriptive name: respondToRequest.

Also, by modifying all the hooks to return the pollyRequest instance, the result that was being returned from the deferred promise will always be the pollyRequest. I guess we can just not have it resolve to anything.

[geigerzaehler]

Either that or give the hook a more descriptive name: respondToRequest.

I prefer this option since it does not require the implementer to call the super method. I’ll update the PR.

Also, by modifying all the hooks to return the pollyRequest instance, the result that was being returned from the deferred promise will always be the pollyRequest. I guess we can just not have it resolve to anything.

Do you mean that we should omit the result argument from pollyRequest.promise.resolve(result) in onRequestFinished()? I have added a commit that fixes that.

[offirgolan]

If you can also squash your commits to a single one and follow the required conventional commit format, that would make sure the build passes. You can do something like: feat: Simplify adapter implementation

[geigerzaehler]

@offirgolan: Thanks for staying with me on this 🙇. Could you also have a look at the updated documentation for implementing a custom adapter that I updated? I hope the PR is now in a mergable state.

[offirgolan]

@geigerzaehler no problem at all! Thank you for seeing this through 😄