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

url: add ToObject method to native URL class #12056

Closed
wants to merge 2 commits into from
Closed

url: add ToObject method to native URL class #12056

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5ceefa
url: add ToObject method to native URL class
jasnell Mar 27, 2017
81236ed
Address feedback
jasnell Mar 27, 2017
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: 2 additions & 0 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@

_process.setupRawDebug();

NativeModule.require('internal/url');
Copy link
Member

Choose a reason for hiding this comment

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

You could add a comment that mentions the side effect you’re using this for, e.g.

// Make sure setURLConstructor() is called before the native
// URL::ToObject() method is used.
NativeModule.require('internal/url');

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit afraid of the increase in memory usage. Is there a need to make some requires in internal/url lazy?

Copy link
Member Author

Choose a reason for hiding this comment

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

There shouldn't be. The requires that are there are minimal (util will have already been loaded, os is small, etc). I'm not convinced this should be a concern.


Object.defineProperty(process, 'argv0', {
enumerable: true,
configurable: false,
Expand Down
27 changes: 27 additions & 0 deletions lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,33 @@ function getPathFromURL(path) {
return isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path);
}

function NativeURL(ctx) {
this[context] = ctx;
}
util.inherits(NativeURL, URL);
Copy link
Member

Choose a reason for hiding this comment

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

I think this would lead to observable differences between URLs constructed with URL vs NativeURL. How about a plain NativeURL.prototype = URL.prototype?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that works too :-)


function constructUrl(flags, protocol, username, password,
host, port, path, query, fragment) {
var ctx = new URLContext();
ctx.flags = flags;
ctx.scheme = protocol;
ctx.username = username;
ctx.password = password;
ctx.port = port;
ctx.path = path;
ctx.query = query;
ctx.fragment = fragment;
ctx.host = host;
const url = new NativeURL(ctx);
if (!url[searchParams]) { // invoked from URL constructor
url[searchParams] = new URLSearchParams();
url[searchParams][context] = this;
Copy link
Member

Choose a reason for hiding this comment

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

Isn’t this === undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, since this function is never "invoked from URL constructor" but !url[searchParams] is always true, you might want to remove the conditional part.

Copy link
Member Author

Choose a reason for hiding this comment

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

heh... yeah, just missed changing this one ;-)

}
initSearchParams(url[searchParams], query);
return url;
}
binding.setURLConstructor(constructUrl);

module.exports = {
toUSVString,
getPathFromURL,
Expand Down
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ namespace node {
V(tls_wrap_constructor_template, v8::FunctionTemplate) \
V(tty_constructor_template, v8::FunctionTemplate) \
V(udp_constructor_function, v8::Function) \
V(url_constructor_function, v8::Function) \
V(write_wrap_constructor_function, v8::Function) \

class Environment;
Expand Down
62 changes: 62 additions & 0 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ using v8::Local;
using v8::Null;
using v8::Object;
using v8::String;
using v8::TryCatch;
using v8::Undefined;
using v8::Value;

Expand Down Expand Up @@ -1418,6 +1419,66 @@ namespace url {
v8::NewStringType::kNormal).ToLocalChecked());
}

MaybeLocal<Value> URL::ToObject(Environment* env) {
Isolate* isolate = env->isolate();
Local<Context> context = env->context();
HandleScope handle_scope(isolate);
Context::Scope context_scope(context);

const Local<Value> undef = Undefined(isolate);
if (context_.flags & URL_FLAGS_FAILED)
return undef;
Copy link
Member

Choose a reason for hiding this comment

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

Either return a Local<Value> as @addaleax suggested, or make this return MaybeLocal<Value>().


Local<Value> argv[9] = {
undef,
undef,
undef,
undef,
undef,
undef,
undef,
undef,
undef,
};
argv[ARG_FLAGS] = Integer::NewFromUnsigned(isolate, context_.flags);
if (context_.flags & URL_FLAGS_HAS_SCHEME)
argv[ARG_PROTOCOL] = OneByteString(isolate, context_.scheme.c_str());
if (context_.flags & URL_FLAGS_HAS_USERNAME)
argv[ARG_USERNAME] = UTF8STRING(isolate, context_.username);
if (context_.flags & URL_FLAGS_HAS_PASSWORD)
argv[ARG_PASSWORD] = UTF8STRING(isolate, context_.password);
if (context_.flags & URL_FLAGS_HAS_HOST)
argv[ARG_HOST] = UTF8STRING(isolate, context_.host);
if (context_.flags & URL_FLAGS_HAS_QUERY)
argv[ARG_QUERY] = UTF8STRING(isolate, context_.query);
if (context_.flags & URL_FLAGS_HAS_FRAGMENT)
argv[ARG_FRAGMENT] = UTF8STRING(isolate, context_.fragment);
if (context_.port > -1)
argv[ARG_PORT] = Integer::New(isolate, context_.port);
if (context_.flags & URL_FLAGS_HAS_PATH)
argv[ARG_PATH] = Copy(env, context_.path);
Copy link
Member

Choose a reason for hiding this comment

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

Can this series of C++-to-JS conversion be somehow consolidated with the code in Parse?

Copy link
Member Author

Choose a reason for hiding this comment

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

A common inline utility method could likely be used for both. I was already considering it :-)


TryCatch try_catch(isolate);

MaybeLocal<Value> ret =
env->url_constructor_function()
->Call(env->context(), undef, 9, argv);

if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(isolate, try_catch);
}

return ret;
Copy link
Member

Choose a reason for hiding this comment

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

If you’re crashing for empty MaybeLocals anyway, you might as well make this method return Local<Value> and pass ret.ToLocalChecked()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't entirely sure what to do here, to be honest. I wanted to get it out so that @bmeck could play with it then tweak it from there. Your suggestion works for me tho :-)

}

static void SetURLConstructor(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_EQ(args.Length(), 1);
CHECK(args[0]->IsFunction());
env->set_url_constructor_function(args[0].As<Function>());
}

static void Init(Local<Object> target,
Local<Value> unused,
Local<Context> context,
Expand All @@ -1428,6 +1489,7 @@ namespace url {
env->SetMethod(target, "toUSVString", ToUSVString);
env->SetMethod(target, "domainToASCII", DomainToASCII);
env->SetMethod(target, "domainToUnicode", DomainToUnicode);
env->SetMethod(target, "setURLConstructor", SetURLConstructor);

#define XX(name, _) NODE_DEFINE_CONSTANT(target, name);
FLAGS(XX)
Expand Down
9 changes: 9 additions & 0 deletions src/node_url.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,18 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "node.h"
#include "env.h"
#include "env-inl.h"

#include <string>

namespace node {
namespace url {

using v8::MaybeLocal;
using v8::Value;


#define BIT_AT(a, i) \
Copy link
Member

Choose a reason for hiding this comment

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

Aside from this PR, maybe we would want to move these private macros and inlined functions into the .cc file itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Likely. It's been on my todo list but just haven't gotten to it yet.

(!!((unsigned int) (a)[(unsigned int) (i) >> 3] & \
(1 << ((unsigned int) (i) & 7))))
Expand Down Expand Up @@ -619,6 +626,8 @@ class URL {
return ret;
}

MaybeLocal<Value> ToObject(Environment* env);
Copy link
Member

Choose a reason for hiding this comment

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

This can be a const method


private:
struct url_data context_;
};
Expand Down