Make Options a parameter that can be passed

[smessie]

This allows function callers to alter the resulting HTTP request instead of only having a subset of hardcoded options.

This PR came out of this discussion where we lacked the ability to set withCredentials: false in the options which is needed to prevent the CORS error in ESS 2.0

[bourgeoa]

This is apparently an ESS issue. @timbl @jeff-zucker

[ThisIsMissEm]

This isn't an ESS issue, but rather, sending requests in Solid with credentials: include is potentially pretty dangerous as you'll be sending all your cookies across to potentially unknown domains whilst resolving resources; Solid doesn't typically use cookies for authentication, instead using OIDC Authorization headers.

A sensible default would be to not include them, and name the option potentiallyDangerouslyIncludeCredentials such that people are aware this could pose a security risk.

For Solid, if the solid server responds to CORS with Access-Control-Allow-Origin: * then you cannot send credentials: include when making the request, as credentials: include must target an explicit origin. There's more information here: https://fetch.spec.whatwg.org/#cors-protocol-and-credentials

[jeff-zucker]

1. I will admit to not totally understanding CORS, but it does seem like there are many circumstances that require credentials=omit. Apparently PATCH on ESS is one of them.

2. Regardless of the CORS issue, there are reasons to allow passing in options to the updater fire method. Currently it is hard-coded :

    let options = {
         noMeta: true,
         contentType: 'application/sparql-update',
         body: query
       }

   @smessie's patch proposes to allow options to be passed in and then do this :

      options.noMeta = true;
       options.contentType = 'application/sparql-update';
       options.body = query;

   I would change those to ||= so that all of them can be overridden. The second one especially since we'll need to support text/n3 PATCH at some point and then the options.contentType will need to be passed in, not hard-coded.

3. There are a bunch of changes in a bunch of files and I haven't yet reviewed the specifics.

[smessie]

I would change those to ||= so that all of them can be overridden. The second one especially since we'll need to support text/n3 PATCH at some point and then the options.contentType will need to be passed in, not hard-coded.

Yes, I was actually considering this while implementing. But then I thought that those are hardcoded now, so that would mean that these functions really want those options configured like that. So because of that I decided to not let them being overwritten by the function caller. Although, maybe we just want to give the ability and the responsibility to the caller and if he breaks something by changing those, it's his responsibility and also in his power to make it work again. So, I'm open to change my PR to make them overwritable by the function caller if you confirm that this would be preferred after all.

[smessie]

Anything I can do to get this PR merged?

[bourgeoa]

Keep it like it for now. An other issue can be open for overwriting options.contentType = 'application/sparql-update'