-
Notifications
You must be signed in to change notification settings - Fork 305
Wire up ResourceMapper #960
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
Wire up ResourceMapper #960
Conversation
Essentially, URLs ending with a '/' will internally be translated to paths such as 'index.html', 'index.ttl', ... depending on the content type
RubenVerborgh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor nits.
lib/resource-mapper.js
Outdated
| // Find a file with the same name (minus the dollar extension) | ||
| const match = files.find(f => this._removeDollarExtension(f) === filename) | ||
| let match = files.find(f => this._removeDollarExtension(f) === filename || | ||
| (isIndex && f.startsWith(this._indexName + '.'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering here if the first case and the isIndex case are mutually exclusive. If they are, we might want to write this differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not, because for example space/index$.html might already be present, so we still should match with that file when needed.
Those files can not be created by solid, following the comment above. But I think it's good to at least be compatible with this case, should those files be created by another application.
| // Error if no match was found, | ||
| // unless the URL ends with a '/', | ||
| // in that case we fallback to the folder itself. | ||
| if (isIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the above comment, we might want fully separate isIndex code paths.
|
Comments are resolved :-) |
(This replaces #959 because I started from a new branch with manual cherry-picking from the old one, instead of doing a highly conflicting rebase)
As requested in #662, this wires up the new ResourceMapper, as the next step for the big ResourceMapper refactor (#946).
A bunch of existing mapping-related issues popped up when working on this PR (see commits), and have also been fixed.