Resource-mapper now considers content type when inserting index.html - #1260
Resource-mapper now considers content type when inserting index.html#1260jaxoncreed wants to merge 1 commit into
Conversation
|
How does this address #1171? |
|
Oops mislabeled it. It's #1120 |
RubenVerborgh
left a comment
RubenVerborgh
left a comment
There was a problem hiding this comment.
While this might hide the behavior of #1120, I don't think this is the proper fix, as it extends ResourceMapper with a piece of logic that seems out of place there.
| match = files.find(f => this._removeDollarExtension(f) === filename) | ||
| // Check if the index file exists | ||
| } else if (files.includes(this._indexFilename)) { | ||
| } else if (files.includes(this._indexFilename) && contentType && contentType.includes(this._indexContentType)) { |
There was a problem hiding this comment.
Hmm. First of all, this can't be correct; the last condition will be true with text/htmlxyz whereas we probably want it to fail. So why the choice for includes here? Is contentType expected to be an Accept header? That would not be a correct use of the parameter, which is intended for a single content type. (Namely in the case of a PUT, where the client sends a Content-Type.)
However, I don't understand the intuition behind this line. What is is supposed to mean? The index file can only be served if the user agent "sends" a content type, that just so happens to be the content type of the index file?
There was a problem hiding this comment.
Perhaps it is, because the contentType can have more values than „text/html” (IIRC a charset parameter or something). That's why an accept match isn't possible.
I had a „starts with” comparison in one of my projects, but a check against MIME types?
https://www.npmjs.com/package/@goa/content-type came after a short search for „RFC 7231”.
There was a problem hiding this comment.
I'd be surprised if a GET could be reduced to a single content type (it cannot), but indeed, content types can have arguments.
| let path, contentType, stats | ||
| try { | ||
| ({ path, contentType } = await this.resourceMapper.mapUrlToFile({ url: options, searchIndex })) | ||
| ({ path, contentType } = await this.resourceMapper.mapUrlToFile({ url: options, contentType: options.contentType, searchIndex })) |
There was a problem hiding this comment.
(See comment below first.)
Is options.contentType the value of the Accept header? Then it's not a content type, so can't be passed. But in any case, passing the content type to the resource mapper is only meaningful in the case of PUT or POST, when a new URL needs to be created; the mapper does not do content negotiation.
It seems to me that the desired logic should be implemented at this level, perhaps as follows:
- Set
searchIndextofalseif theAcceptheader does not explicitly ask fortext/html. - Then call
mapUrlToFile.
There was a problem hiding this comment.
Ah okay thanks. I'll do that
|
Turns out this is much more complicated than setting https://github.com/solid/node-solid-server/blob/master/lib/handlers/get.js#L48 to
I will come back to this one later. |
We should probably document what exactly it does 🙂
That is independent of
We should avoid that; ResourceMapper is content-type-agnostic at the moment. |
I might be interpreting this incorrectly, but I believe that https://github.com/solid/node-solid-server/blob/master/lib/resource-mapper.js#L119 says that in order to find files with the $ extension, search index must be true. Is this the way it should work? |
|
I'm closing this PR in favour of the one proposed here #1282 |
3 participants
Fix for #1120 again