refactor: replace url.resolve() with String.replace()#4479
refactor: replace url.resolve() with String.replace()#4479curbengh wants to merge 2 commits intohexojs:masterfrom
Conversation
|
Should we bring up a |
| // resolve `path`'s absolute path relative to current page's url | ||
| // `path` can be both absolute (starts with `/`) or relative. | ||
| // `path` can be both absolute (starts with `http://`) or relative (starts with `/`). | ||
| return resolve(url || config.url, path); |
There was a problem hiding this comment.
Should we bring up a resolveURL in hexo-util?
L173 could be replaced with:
const urlVal = url || config.url;
return urlVal.replace(/\/$/, '') + '/' + path.replace(urlVal, '').replace(/\\/g, '/').replace(/^\//, '')but it only works when both urlVal and path have the same protocol scheme (e.g. both start with http:// or both start with //).
url.resolve() is (surprisingly) robust, I tested:
http://+////+http://http://+https://
and the output is still accurate.
in this "open_graph.js", url.resolve() is even robust to when path variable is an external link, in which it returns external link instead.
Other plugins (of this PR) only deal with relative path, not absolute url, so (a + b).replace(/\\/g, '/').replace(/\/{2,}/g, '/') works fine. Perhaps can implement joinPath()?
Also, in other plugins (of this PR), url.resolve() is actually not used according to its purpose, currently it's used as path.join() plus converting backward to forward slash, whereas this L123 does properly utilise url.resolve().
| if (escape === 'true') title = attrTitle; | ||
|
|
||
| const link = encodeURL(resolve(ctx.config.root, asset.path)); | ||
| const link = encodeURL((ctx.config.root + asset.path).replace(/\\/g, '/').replace(/\/{2,}/g, '/')); |
There was a problem hiding this comment.
url_for() currently cannot be used here because it doesn't convert backward to forward slash.
There was a problem hiding this comment.
How to testgit clone -b resolve-path-string-replace https://github.com/curbengh/hexo.git
cd hexo
npm install
npm test |
4 participants
What does it do?
url.resolve()is a legacy API, so it's best to avoid it if possible,String.replace()is also 6x faster.How to test
Screenshots
Pull request tasks