Skip to content
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.

Sign up for GitHub

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

refactor: replace url.resolve() with String.replace() #4479

Closed
wants to merge 2 commits into from
Closed

refactor: replace url.resolve() with String.replace() #4479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b94846d
refactor: replace url.resolve() with String.replace()
curbengh Aug 15, 2020
27384cc
docs(open_graph): fix comment
curbengh Aug 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/plugins/helper/open_graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ function openGraphHelper(options = {}) {
images = images.map(path => {
if (!parse(path).host) {
// 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);
Copy link
Contributor Author

@curbengh curbengh Aug 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. http:// + //
  2. // + http://
  3. 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().

}

Expand Down
3 changes: 1 addition & 2 deletions lib/plugins/tag/asset_img.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';

const { resolve } = require('url');
const img = require('./img');
const { encodeURL } = require('hexo-util');

Expand All @@ -20,7 +19,7 @@ module.exports = ctx => {
for (let i = 0; i < len; i++) {
const asset = PostAsset.findOne({post: this._id, slug: args[i]});
if (asset) {
args[i] = encodeURL(resolve('/', asset.path));
args[i] = encodeURL(('/' + asset.path).replace(/\\/g, '/').replace(/\/{2,}/g, '/'));
return img(ctx)(args);
}
}
Expand Down
3 changes: 1 addition & 2 deletions lib/plugins/tag/asset_link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const { encodeURL, escapeHTML } = require('hexo-util');
const { resolve } = require('url');

/**
* Asset link tag
Expand Down Expand Up @@ -30,7 +29,7 @@ module.exports = ctx => {
const attrTitle = escapeHTML(title);
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, '/'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url_for() currently cannot be used here because it doesn't convert backward to forward slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hexojs/hexo-util#228


return `<a href="${link}" title="${attrTitle}">${title}</a>`;
};
Expand Down
3 changes: 1 addition & 2 deletions lib/plugins/tag/asset_path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';

const { resolve } = require('url');
const { encodeURL } = require('hexo-util');

/**
Expand All @@ -19,7 +18,7 @@ module.exports = ctx => {
const asset = PostAsset.findOne({post: this._id, slug});
if (!asset) return;

const path = encodeURL(resolve(ctx.config.root, asset.path));
const path = encodeURL((ctx.config.root + asset.path).replace(/\\/g, '/').replace(/\/{2,}/g, '/'));

return path;
};
Expand Down
3 changes: 1 addition & 2 deletions lib/plugins/tag/post_link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const { encodeURL, escapeHTML } = require('hexo-util');
const { resolve } = require('url');

/**
* Post link tag
Expand Down Expand Up @@ -31,7 +30,7 @@ module.exports = ctx => {
const attrTitle = escapeHTML(title);
if (escape === 'true') title = attrTitle;

const link = encodeURL(resolve(ctx.config.root, post.path));
const link = encodeURL((ctx.config.root + post.path).replace(/\\/g, '/').replace(/\/{2,}/g, '/'));

return `<a href="${link}" title="${attrTitle}">${title}</a>`;
};
Expand Down
3 changes: 1 addition & 2 deletions lib/plugins/tag/post_path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict';

const { resolve } = require('url');
const { encodeURL } = require('hexo-util');

/**
Expand All @@ -19,7 +18,7 @@ module.exports = ctx => {
const post = Post.findOne({slug});
if (!post) return;

const link = encodeURL(resolve(ctx.config.root, post.path));
const link = encodeURL((ctx.config.root + post.path).replace(/\\/g, '/').replace(/\/{2,}/g, '/'));

return link;
};
Expand Down