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

Merged
merged 3 commits into from
Jul 17, 2019

Conversation

tombh
Copy link
Contributor

@tombh tombh 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
Copy link
Contributor Author

tombh commented Jun 29, 2019

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

@offirgolan
Copy link
Collaborator

@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.

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 mapboxgl-fixes branch from 05f38ad to 7c11ada Compare July 4, 2019 06:38
@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
Copy link
Contributor Author

tombh 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
Copy link
Contributor Author

tombh 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
Copy link
Collaborator

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.

@offirgolan offirgolan added 📦 adapter-fetch bug 🐞 Something isn't working labels Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working 📦 adapter-fetch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants