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.

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

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 27, 2017

Provides a factory method to convert a native URL class into a JS URL object.

Environment* env = ...

URL url("http://example.org/a/b/c?query#fragment");

MaybeLocal<Value> val = url.ToObject(env);

(Including a test for this is a bit difficult at the current time.)

/cc @bmeck

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

Provides a factory method to convert a native URL class
into a JS URL object.

```c++
Environment* env = ...

URL url("http://example.org/a/b/c?query#fragment");

MaybeLocal<Value> val = url.ToObject(env);
```
@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 27, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x lib / src Issues and PRs related to general changes in the lib or src directory. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 27, 2017
@jasnell jasnell requested a review from bmeck March 27, 2017 02:59
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 ;-)

@@ -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');

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 :-)

src/node_url.cc Outdated
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 :-)

src/node_url.h Outdated
@@ -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

src/node_url.cc Outdated
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 :-)

src/node_url.cc Outdated

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>().

@@ -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.

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.

src/node_url.h Outdated
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.

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.

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

@jasnell
Copy link
Member Author

jasnell commented Mar 27, 2017

Updated! PTAL!

I do not consider this to be ready to land yet, btw. I'd like feedback from @bmeck on whether this will work for what he needs before landing. Marking as in progress so it's not landed prematurely.

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Mar 27, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This PR LGTM in its current state

src/node_url.h Outdated
@@ -626,7 +626,7 @@ class URL {
return ret;
}

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

Choose a reason for hiding this comment

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

Specifying const on a return type dosn’t really do anything, btw ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Bad habits die hard. I appreciate the reminder. Eventually I'll remember ;)

@jasnell
Copy link
Member Author

jasnell commented Mar 27, 2017

jasnell added a commit that referenced this pull request Mar 30, 2017
Provides a factory method to convert a native URL class
into a JS URL object.

```c++
Environment* env = ...

URL url("http://example.org/a/b/c?query#fragment");

MaybeLocal<Value> val = url.ToObject(env);
```

PR-URL: #12056
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Mar 30, 2017

Landed in 7139b93

@italoacasas
Copy link
Contributor

cc @jasnell

@TimothyGu TimothyGu removed wip Issues and PRs that are still a work in progress. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 19, 2017
@TimothyGu
Copy link
Member

Removing semver-minor since this API is not exposed to users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants