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

Mapbox GL fixes: handle `Request` objects as URLs (and array buffer responses discussion) #220

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@tombh
Copy link
Contributor

commented Jun 29, 2019

Description

These are fixes I needed to get Polly working with Mapbox GL. So far the handling of an array buffer response is only for passthrough requests, not recording, is that a reasonable first step? There are no tests because I'd like some help thinking about how to test these.

Motivation and Context

I wonder if the handling of Request objects for URLs has already been considered? It's just that the the coercion of those objects into a URL string isn't handled simply by casting to a string?

I see in #183 that array buffer responses have already been considered.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My code follows the code style of this project.
  • My commits and the title of this PR follow the Conventional Commits Specification.
  • I have read the contributing guidelines.
@tombh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

I think those test failures are linting errors on files not changed in this PR?

@offirgolan

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

@tombh thanks for taking the time in opening up this PR!

I wonder if the handling of Request objects for URLs has already been considered? It's just that the the coercion of those objects into a URL string isn't handled simply by casting to a string?

This is definitely something that we should handle and should've been considered but I guess was missed. Can you split this into a separate PR so we can get this in faster as the ArrayBuffer work is going to be a bit more time consuming.

One note: PollyRequest does expect url to be a string or an object that can be casted into a url string. The logic to pass the correct url from a Request instance should be handled in the fetch adapter layer instead.

So far the handling of an array buffer response is only for passthrough requests, not recording, is that a reasonable first step?

Due to the current nature of the client server API (polly.server) even though we only target supporting binary data in passthroughs, it still flows through all the server route handlers and middleware. This can cause a pretty trolling behavior since the response body will be different depending on the recording mode.

Im not exactly sure how you want to proceed with this but supporting binary data would need to work across all recording modes in order to be merged in.

feat(core): Accept `Request` objects as URLs
As well as WHATWG's URL objects it's possible to receive `Request`
objects as 'URLs'. So handle such cases by extracting the URL property.

@tombh tombh force-pushed the tombh:mapboxgl-fixes branch from 05f38ad to 7c11ada Jul 4, 2019

@tombh tombh changed the title [WIP] Mapbox GL fixes: handle `Request` objects as URLs and array buffer responses Mapbox GL fixes: handle `Request` objects as URLs (and array buffer responses discussion) Jul 4, 2019

@tombh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Ok I've removed the ArrayBuffer fix and added a test for the Request object fix. BTW is there not a way to run a single test? My laptop quite struggles with testem.

@tombh

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Again tests seem to fail because of an unassociated linting errors. But in fact I don't even seem to be able to see my added test in the logs.

@offirgolan

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2019

Thanks for taking the time to land this @tombh. The CI failures are unrelated, we'll fix them separately.

BTW is there not a way to run a single test? My laptop quite struggles with testem.

Yeah, the current test/build system needs some work for sure. You can run a single test by using it.only on the test and then running yarn test -l Chrome or yarn test:ci -l Chrome.

@@ -82,6 +82,9 @@ export default class PollyRequest extends HTTPBase {
}

set url(value) {
if (typeof value === 'object' && 'url' in value) {

This comment has been minimized.

Copy link
@offirgolan

offirgolan Jul 5, 2019

Collaborator

This logic should be moved into the fetch adapter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.