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

Jump to bottom

url: simplify and improve url formatting #46736

Merged
merged 1 commit into from
Feb 24, 2023
+111 −110
Merged

url: simplify and improve url formatting #46736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 13 additions & 85 deletions lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ const path = require('path');

const {
validateFunction,
validateObject,
} = require('internal/validators');

const querystring = require('querystring');
Expand Down Expand Up @@ -150,8 +149,6 @@ class URLContext {
password = '';
port = '';
hash = '';
hasHost = false;
hasOpaquePath = false;
}

function isURLSearchParams(self) {
Expand Down Expand Up @@ -282,7 +279,9 @@ class URLSearchParams {
name = toUSVString(name);
value = toUSVString(value);
ArrayPrototypePush(this[searchParams], name, value);
update(this[context], this);
if (this[context]) {
this[context].search = this.toString();
}
}

delete(name) {
Expand All @@ -303,7 +302,9 @@ class URLSearchParams {
i += 2;
}
}
update(this[context], this);
if (this[context]) {
this[context].search = this.toString();
}
}

get(name) {
Expand Down Expand Up @@ -398,7 +399,9 @@ class URLSearchParams {
ArrayPrototypePush(list, name, value);
}

update(this[context], this);
if (this[context]) {
this[context].search = this.toString();
}
}

sort() {
Expand Down Expand Up @@ -442,7 +445,9 @@ class URLSearchParams {
}
}

update(this[context], this);
if (this[context]) {
this[context].search = this.toString();
anonrig marked this conversation as resolved.
Show resolved Hide resolved
}
}

// https://heycam.github.io/webidl/#es-iterators
Expand Down Expand Up @@ -528,46 +533,6 @@ function isURLThis(self) {
return (self !== undefined && self !== null && self[context] !== undefined);
}

function constructHref(ctx, options) {
if (options)
validateObject(options, 'options');

options = {
fragment: true,
unicode: false,
search: true,
auth: true,
...options
};

// https://url.spec.whatwg.org/#url-serializing
let ret = ctx.protocol;
if (ctx.hasHost) {
ret += '//';
const hasUsername = ctx.username !== '';
const hasPassword = ctx.password !== '';
if (options.auth && (hasUsername || hasPassword)) {
if (hasUsername)
ret += ctx.username;
if (hasPassword)
ret += `:${ctx.password}`;
ret += '@';
}
ret += options.unicode ?
domainToUnicode(ctx.hostname) : ctx.hostname;
if (ctx.port !== '')
ret += `:${ctx.port}`;
} else if (!ctx.hasOpaquePath && ctx.pathname.lastIndexOf('/') !== 0 && ctx.pathname.startsWith('//')) {
ret += '/.';
}
ret += ctx.pathname;
if (options.search)
ret += ctx.search;
if (options.fragment)
ret += ctx.hash;
return ret;
}

class URL {
constructor(input, base = undefined) {
// toUSVString is not needed.
Expand Down Expand Up @@ -620,14 +585,8 @@ class URL {
return `${constructor.name} ${inspect(obj, opts)}`;
}

[kFormat](options) {
// TODO(@anonrig): Replace kFormat with actually calling setters.
return constructHref(this[context], options);
}

#onParseComplete = (href, origin, protocol, hostname, pathname,
search, username, password, port, hash, hasHost,
hasOpaquePath) => {
search, username, password, port, hash) => {
const ctx = this[context];
ctx.href = href;
ctx.origin = origin;
Expand All @@ -639,9 +598,6 @@ class URL {
ctx.password = password;
ctx.port = port;
ctx.hash = hash;
// TODO(@anonrig): Remove hasHost and hasOpaquePath when kFormat is removed.
ctx.hasHost = hasHost;
ctx.hasOpaquePath = hasOpaquePath;
if (!this[searchParams]) { // Invoked from URL constructor
this[searchParams] = new URLSearchParams();
this[searchParams][context] = this;
Expand Down Expand Up @@ -854,33 +810,6 @@ ObjectDefineProperties(URL, {
revokeObjectURL: kEnumerableProperty,
});

function update(url, params) {
if (!url)
return;

const ctx = url[context];
const serializedParams = params.toString();
if (serializedParams.length > 0) {
ctx.search = '?' + serializedParams;
} else {
ctx.search = '';

// Potentially strip trailing spaces from an opaque path
if (ctx.hasOpaquePath && ctx.hash.length === 0) {
let length = ctx.pathname.length;
while (length > 0 && ctx.pathname.charCodeAt(length - 1) === 32) {
length--;
}

// No need to copy the whole string if there is no space at the end
if (length !== ctx.pathname.length) {
ctx.pathname = ctx.pathname.slice(0, length);
}
}
}
ctx.href = constructHref(ctx);
}

function initSearchParams(url, init) {
if (!init) {
url[searchParams] = [];
Expand Down Expand Up @@ -1379,7 +1308,6 @@ module.exports = {
domainToASCII,
domainToUnicode,
urlToHttpOptions,
formatSymbol: kFormat,
searchParamsSymbol: searchParams,
encodeStr
};
46 changes: 38 additions & 8 deletions lib/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'use strict';

const {
Boolean,
Int8Array,
ObjectKeys,
SafeSet,
Expand All @@ -37,7 +38,10 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_URL,
} = require('internal/errors').codes;
const { validateString } = require('internal/validators');
const {
validateString,
validateObject,
} = require('internal/validators');

// This ensures setURLConstructor() is called before the native
// URL::ToObject() method is used.
Expand All @@ -50,11 +54,14 @@ const {
domainToASCII,
domainToUnicode,
fileURLToPath,
formatSymbol,
pathToFileURL,
urlToHttpOptions,
} = require('internal/url');

const {
formatUrl,
} = internalBinding('url');

// Original url.parse() API

function Url() {
Expand Down Expand Up @@ -587,13 +594,36 @@ function urlFormat(urlObject, options) {
} else if (typeof urlObject !== 'object' || urlObject === null) {
throw new ERR_INVALID_ARG_TYPE('urlObject',
['Object', 'string'], urlObject);
} else if (!(urlObject instanceof Url)) {
const format = urlObject[formatSymbol];
return format ?
format.call(urlObject, options) :
Url.prototype.format.call(urlObject);
} else if (urlObject instanceof URL) {
let fragment = true;
let unicode = false;
let search = true;
let auth = true;

if (options) {
validateObject(options, 'options');

if (options.fragment != null) {
fragment = Boolean(options.fragment);
}

if (options.unicode != null) {
unicode = Boolean(options.unicode);
}

if (options.search != null) {
search = Boolean(options.search);
}

if (options.auth != null) {
auth = Boolean(options.auth);
}
Copy link
Member

Choose a reason for hiding this comment

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

Setting these up as a set of bit flags may be a bit nicer.

e.g. something like

let flags = 0;
if (options.fragment != null) flags |= kFragment;
if (options.unicode != null) flags |= kUnicode;
// ...

Copy link
Member Author

Choose a reason for hiding this comment

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

That does not cover passing { auth: 1 } as options, and still requires Boolean(). I'm not personally in favor of this solution.

}

return formatUrl(urlObject.href, fragment, unicode, search, auth);
}
return urlObject.format();

return Url.prototype.format.call(urlObject);
}

// These characters do not need escaping:
Expand Down
73 changes: 59 additions & 14 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace node {

using v8::Boolean;
using v8::Context;
using v8::Function;
using v8::FunctionCallbackInfo;
Expand Down Expand Up @@ -47,7 +46,7 @@ enum url_update_action {
kHref = 9,
};

void SetArgs(Environment* env, Local<Value> argv[12], const ada::result& url) {
void SetArgs(Environment* env, Local<Value> argv[10], const ada::result& url) {
Isolate* isolate = env->isolate();
argv[0] = Utf8String(isolate, url->get_href());
argv[1] = Utf8String(isolate, url->get_origin());
Expand All @@ -59,8 +58,6 @@ void SetArgs(Environment* env, Local<Value> argv[12], const ada::result& url) {
argv[7] = Utf8String(isolate, url->get_password());
argv[8] = Utf8String(isolate, url->get_port());
argv[9] = Utf8String(isolate, url->get_hash());
argv[10] = Boolean::New(isolate, url->host.has_value());
argv[11] = Boolean::New(isolate, url->has_opaque_path);
}

void Parse(const FunctionCallbackInfo<Value>& args) {
Expand All @@ -86,8 +83,7 @@ void Parse(const FunctionCallbackInfo<Value>& args) {
}
base_pointer = &base.value();
}
ada::result out =
ada::parse(std::string_view(input.out(), input.length()), base_pointer);
ada::result out = ada::parse(input.ToStringView(), base_pointer);

if (!out) {
return args.GetReturnValue().Set(false);
Expand All @@ -105,8 +101,6 @@ void Parse(const FunctionCallbackInfo<Value>& args) {
undef,
undef,
undef,
undef,
undef,
};
SetArgs(env, argv, out);
USE(success_callback_->Call(
Expand Down Expand Up @@ -192,10 +186,8 @@ void UpdateUrl(const FunctionCallbackInfo<Value>& args) {
Utf8Value new_value(isolate, args[2].As<String>());
Local<Function> success_callback_ = args[3].As<Function>();

std::string_view new_value_view =
std::string_view(new_value.out(), new_value.length());
std::string_view input_view = std::string_view(input.out(), input.length());
ada::result out = ada::parse(input_view);
std::string_view new_value_view = new_value.ToStringView();
ada::result out = ada::parse(input.ToStringView());
CHECK(out);

bool result{true};
Expand Down Expand Up @@ -255,21 +247,73 @@ void UpdateUrl(const FunctionCallbackInfo<Value>& args) {
undef,
undef,
undef,
undef,
undef,
};
SetArgs(env, argv, out);
USE(success_callback_->Call(
env->context(), args.This(), arraysize(argv), argv));
args.GetReturnValue().Set(result);
}

void FormatUrl(const FunctionCallbackInfo<Value>& args) {
CHECK_GT(args.Length(), 4);
CHECK(args[0]->IsString()); // url href

Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

Utf8Value href(isolate, args[0].As<String>());
const bool fragment = args[1]->IsTrue();
const bool unicode = args[2]->IsTrue();
const bool search = args[3]->IsTrue();
const bool auth = args[4]->IsTrue();

ada::result out = ada::parse(href.ToStringView());
CHECK(out);

if (!fragment) {
out->fragment = std::nullopt;
}

if (unicode) {
#if defined(NODE_HAVE_I18N_SUPPORT)
std::string hostname = out->get_hostname();
MaybeStackBuffer<char> buf;
int32_t len = i18n::ToUnicode(&buf, hostname.data(), hostname.length());

if (len < 0) {
out->host = "";
} else {
out->host = buf.ToString();
}
#else
out->host = "";
#endif
}

if (!search) {
out->query = std::nullopt;
}

if (!auth) {
out->username = "";
out->password = "";
}

std::string result = out->get_href();
args.GetReturnValue().Set(String::NewFromUtf8(env->isolate(),
result.data(),
NewStringType::kNormal,
result.length())
.ToLocalChecked());
}

void Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
SetMethod(context, target, "parse", Parse);
SetMethod(context, target, "updateUrl", UpdateUrl);
SetMethod(context, target, "formatUrl", FormatUrl);

SetMethodNoSideEffect(context, target, "domainToASCII", DomainToASCII);
SetMethodNoSideEffect(context, target, "domainToUnicode", DomainToUnicode);
Expand All @@ -279,6 +323,7 @@ void Initialize(Local<Object> target,
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(Parse);
registry->Register(UpdateUrl);
registry->Register(FormatUrl);

registry->Register(DomainToASCII);
registry->Register(DomainToUnicode);
Expand Down