Optional fetch with credentials

[mcclurec]

Description

Follow up to PR 291. Adding credentials: 'include' to all fetch calls had unexpected CORS side effects for users. This PR introduces a global mixin to expose a fetchOptions helper method on all components. "Service" items can now include a fetchWithCredentials prop on their config entry which will add the fetch option as needed.

Fixes # Issues 296 and 297

Type of change

• 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 not work as expected)

Checklist:

• I've read & comply with the contributing guidelines
• I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
• I have made corresponding changes the documentation (README.md).
• I've checked my modifications for any breaking changes, especially in the config.yml file

[tobidemski]

LGTM.

One question. Maybe we should reconsider in the next major version how to define some header values?
Maybe one option on each service with the name "headers" and you can set your own values. It would be totally generic but for each service the same instead each service provides own options like apiKey...

Thanks for this update!

[bastienwirtz]

Hi there! Thanks for the contribution!

@mcclurec @D4rkiii I'm working on improving the custom services dev and I pushed the first part earlier.

I'm currently working to add a global mixin that, among other things, provides a fetch method with some pre-configured option like that. @mcclurec I will integrate your work in it if you are ok with it ?

[bastienwirtz]

Hi there !

@mcclurec I just pushed the mixing I had in mind, and refoactored ping and AdGuard service to use it (and all other services as soon as I find the time).

What do you think? Let me know if you have any question (I will improve the documentation about that soon).

[mcclurec]

@bastienwirtz I like it. I initially though about wrapping all of fetch but wasn't sure if that was a bridge too far.

My one piece of feedback is to make it as faithful a reproduction of the native fetch API as possible. That way, when people come along in the future, they can read MDN docs or whatever and have it behave as expected. No extra magic beyond exactly what we need. That means two things in my mind...

1. Let's not add the optional third json param
2. Don't do any clever path manipulation stuff. Just have the implementor pass in the full url they want. Personally, I'd rather have them construct something like ${this.endpoint}/${path} in the service component rather than have the magic hidden in the mixin.

Feel free to close this PR in favor of your own

[mcclurec]

@bastienwirtz I refactored my PR to utilize the service mixin's fetch. I removed the json boolean trap and the endpoint property in favor of a more transparent implementation of fetch.

Let me know if you have strong opinions about either of those choices. My thought is that it would be better to get this out in the community now and fix the CORS issues, and if we later want to refactor the mixin to handle more of the data fetching nitty gritty, we can do that in a separate PR.

[bastienwirtz]

Hey @mcclurec,

The mixin is a shortcut, and even if it's 100% compatible with the native fetch signature, it's not meant to be a generic replacement. It provides the error checking and json decoding, which is useful for ALL services so far (with the boolean to handle specific case, and beyond that you the native fetch is still here if something), that's our use case here. It helps standardize all services implementation and ease the custom service development.
So I rather keep the first implementation. I'll push the refactoring of the other services to use the internal fetch asap.

[bastienwirtz]

All right, I finally pushed the last parts of the re-factorization to use the internal fetch method & the Generic template as a base. Credential include is now optional and service code is much smaller. We can close this issue.
Thanks again @mcclurec for your work on this.