Add headers option support to other loaders #176

lutangar · 2017-04-27T15:53:29Z

Hi there,

Pretty self-explanatory, I didn't expect to find the headers options to be node loader only...
The support of this option is now extended to jquery and xhr loaders.

I may work on a fetch loader in the future, with the approriate polyfill for node, that would mean same code for node and the browser and a promise first API.

davidlehn · 2017-04-27T17:01:11Z

js/jsonld.js

+/**
+ * Build an headers object from custom headers and assert Accept header is not override.
+ *
+ * @param headers an o


dlongley · 2017-04-27T18:01:49Z

js/jsonld.js

@@ -1646,6 +1646,30 @@ jsonld.cache = {
 };

 /**
+ * Accept header.
+ */
+jsonld.acceptHeader = 'application/ld+json, application/json';


We should really have a jsonld.defaults object to put things like this under, if we don't have that already. If we don't have it, let's add it now before adding yet another new top-level thing:

jsonld.defaults = { headers: { accept: 'application/ld+json, application/json' } };

I wonder if it needs further scoping than that even. Thoughts from anyone else?

If this isn't a default that could be changed or if we're not into figuring this out right now, let's not expose it in the public API.

This isn't a default, I just reproduced the behaviour previously implemented on the node loader.

But in my opinion, it should only be a default. The current implementation is far too restrictive on this accept header. The RangeError exception should only be raised when the accept header doesn't contain application/ld+json nor application/json.

This way it could be overridden for more exotic things like application/ld+json, text/html, application/json, that would still be valid.

What do you think?

Also agreed on the defaults object, if that looks good to you.

I recommend we just take acceptHeader off of the public API for now (off of jsonld.) and make a private module defaults var:

var _defaults = { headers: { accept: 'application/ld+json, application/json' } };

That way we move towards something we can expose in the future but we don't have to decide on exactly what it's shape is since that's not the point of this PR right now.

Alright, will do, but keep in mind this isn't exactly a default value at the moment, since it can't be changed.

Yup, it's just a private marker for a half-baked idea.

dlongley · 2017-04-28T14:49:34Z

js/jsonld.js

@@ -1646,6 +1646,30 @@ jsonld.cache = {
 };

 /**
+ * Accept header.
+ */
+jsonld.acceptHeader = 'application/ld+json, application/json';


I recommend we just take acceptHeader off of the public API for now (off of jsonld.) and make a private module defaults var:

var _defaults = { headers: { accept: 'application/ld+json, application/json' } };

That way we move towards something we can expose in the future but we don't have to decide on exactly what it's shape is since that's not the point of this PR right now.

dlongley · 2017-04-28T14:58:18Z

js/jsonld.js

+ */
+jsonld.buildHeaders = function (headers) {
+  headers = headers || {};
+  if ('Accept' in headers || 'accept' in headers) {


Nits: Remove space after if and function to keep in the existing code style of this module.

Also, this check here will cover only two possible capitalizations of accept. It's annoying, but perhaps we should do this instead:

var hasAccept = Object.keys(headers).map(function(h) { return h.toLowerCase(); }).indexOf('accept') !== -1; if(hasAccept) { // throw range error }

dlongley · 2017-04-28T14:59:13Z

js/jsonld.js

@@ -1656,6 +1680,8 @@ jsonld.documentLoaders = {};
 * @param $ the jquery instance to use.
 * @param options the options to use:
 *          secure: require all URLs to use HTTPS.
+ *          headers: an array of headers which will be passed as request


headers is actually not an array, but an object (map), right?

dlongley

LGTM

lutangar · 2017-05-01T13:54:50Z

Should I be concerned about these tests failing? It seems master also have these errors...
https://travis-ci.org/digitalbazaar/jsonld.js/jobs/226858807

1) JSON-LD JSON-LD Test Suite Error handling #tp002 If not specified using processingMode, processing mode is taken as json-ld-1.0:
    AssertionError: 'invalid term definition' == 'processing mode conflict'
2) JSON-LD JSON-LD Test Suite Error handling #tp003 If @version is specified, it must be 1.1:
    AssertionError: 'invalid term definition' == 'invalid @version value'
3) JSON-LD JSON-LD Test Suite Error handling #tp005 If not specified using processingMode, processing mode is taken as json-ld-1.0:
    AssertionError: 'invalid term definition' == 'processing mode conflict'
4) JSON-LD JSON-LD Test Suite Error handling #tp006 If @version is specified, it must be 1.1:
    AssertionError: 'invalid term definition' == 'invalid @version value'

dlongley · 2017-05-01T13:58:02Z

@lutangar,

No those tests are an ongoing issue right now (#172), you can safely ignore them. It looks like the other tests pass, so you're good.

dlongley · 2017-05-01T13:59:06Z

@davidlehn, can you please review to see if we're good to accept this? Thanks!

davidlehn · 2017-05-01T17:47:01Z

jsonld.buildHeaders should probably just be a local function for now instead of being exposed on jsonld. Does it need to be a public API?

jsonld.buildHeaders is mutating the input options and adding the 'Accept' field. This would be a problem if you call the APIs with the same options object twice and the code checking for 'Accept' would throw an error the second time. Can fix this by copying all the options headers over into a new headers object (like some of the code did before).

lutangar · 2017-05-02T08:42:22Z

@davidlehn buildHeaders doesn't need to be public at the moment, this has been changed.
I also changed back headers construction to something closer to previous code, meaning without the mutation.

lutangar · 2017-05-09T09:40:22Z

@dlongley @davidlehn is there anything else I could do to help this to be merged?

dlongley · 2017-05-09T14:56:39Z

@lutangar, I don't think so, it LGTM. @davidlehn, can we pull this in now?

davidlehn · 2017-05-09T17:04:06Z

Looks ok. I'll rebase and merge. I'm going to add a patch to use some() vs map(). Not a big deal in this case but might as well: https://jsperf.com/check-for-lowercase-key/

davidlehn reviewed Apr 27, 2017

View reviewed changes

dlongley reviewed Apr 27, 2017

View reviewed changes

dlongley requested changes Apr 28, 2017

View reviewed changes

lutangar force-pushed the feature/loader-headers-options branch from 51440e3 to 7833bc2 Compare April 28, 2017 15:26

dlongley approved these changes Apr 28, 2017

View reviewed changes

add headers option support to other loaders

28014d9

lutangar force-pushed the feature/loader-headers-options branch from 7833bc2 to 28014d9 Compare May 2, 2017 08:38

davidlehn approved these changes May 9, 2017

View reviewed changes

davidlehn merged commit 22dac47 into digitalbazaar:master May 9, 2017

lutangar mentioned this pull request May 12, 2017

Allow valid accept headers in document loader options #182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add headers option support to other loaders #176

Add headers option support to other loaders #176

lutangar commented Apr 27, 2017

davidlehn Apr 27, 2017

dlongley Apr 27, 2017

dlongley Apr 27, 2017

lutangar Apr 27, 2017 •

edited

Loading

lutangar Apr 27, 2017

dlongley Apr 28, 2017

lutangar Apr 28, 2017

dlongley Apr 28, 2017

dlongley Apr 28, 2017

dlongley Apr 28, 2017

dlongley Apr 28, 2017

dlongley left a comment

lutangar commented May 1, 2017

dlongley commented May 1, 2017 •

edited

Loading

dlongley commented May 1, 2017

davidlehn commented May 1, 2017

lutangar commented May 2, 2017

lutangar commented May 9, 2017 •

edited

Loading

dlongley commented May 9, 2017

davidlehn commented May 9, 2017

Add headers option support to other loaders #176

Add headers option support to other loaders #176

Conversation

lutangar commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lutangar Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongley left a comment

Choose a reason for hiding this comment

lutangar commented May 1, 2017

dlongley commented May 1, 2017 • edited Loading

dlongley commented May 1, 2017

davidlehn commented May 1, 2017

lutangar commented May 2, 2017

lutangar commented May 9, 2017 • edited Loading

dlongley commented May 9, 2017

davidlehn commented May 9, 2017

lutangar Apr 27, 2017 •

edited

Loading

dlongley commented May 1, 2017 •

edited

Loading

lutangar commented May 9, 2017 •

edited

Loading