url: add ToObject method to native URL class by jasnell · Pull Request #12056 · nodejs/node

jasnell · 2017-03-27T02:59:23Z

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); ```

addaleax · 2017-03-27T04:10:25Z

lib/internal/url.js

+  const url = new NativeURL(ctx);
+  if (!url[searchParams]) { // invoked from URL constructor
+    url[searchParams] = new URLSearchParams();
+    url[searchParams][context] = this;


Isn’t this === undefined?

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

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

addaleax · 2017-03-27T04:10:56Z

lib/internal/bootstrap_node.js


    _process.setupRawDebug();

+    NativeModule.require('internal/url');


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

addaleax · 2017-03-27T04:15:14Z

lib/internal/url.js

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


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

Yep, that works too :-)

addaleax · 2017-03-27T04:18:43Z

src/node_url.cc

+      FatalException(isolate, try_catch);
+    }
+
+    return ret;


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

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

addaleax · 2017-03-27T04:20:35Z

src/node_url.h

    return ret;
  }

+  MaybeLocal<Value> ToObject(Environment* env);


This can be a const method

TimothyGu · 2017-03-27T07:07:11Z

src/node_url.cc

+    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);


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

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

TimothyGu · 2017-03-27T07:07:56Z

src/node_url.cc

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


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

TimothyGu · 2017-03-27T07:09:06Z

lib/internal/bootstrap_node.js


    _process.setupRawDebug();

+    NativeModule.require('internal/url');


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

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.

TimothyGu · 2017-03-27T07:14:42Z

src/node_url.h

+using v8::Value;
+
+
 #define BIT_AT(a, i)                                                          \


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

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

TimothyGu · 2017-03-27T07:20:36Z

lib/internal/url.js

+  const url = new NativeURL(ctx);
+  if (!url[searchParams]) { // invoked from URL constructor
+    url[searchParams] = new URLSearchParams();
+    url[searchParams][context] = this;


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 · 2017-03-27T15:11:29Z

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.

addaleax

This PR LGTM in its current state

addaleax · 2017-03-27T15:24:30Z

src/node_url.h

  }

-  MaybeLocal<Value> ToObject(Environment* env);
+  const Local<Value> ToObject(Environment* env) const;


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

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

jasnell · 2017-03-27T15:38:19Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7059/

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 <anna@addaleax.net>

jasnell · 2017-03-30T03:57:03Z

Landed in 7139b93

italoacasas · 2017-04-10T17:53:50Z

cc @jasnell

TimothyGu · 2017-04-19T07:50:16Z

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

url: add ToObject method to native URL class

f5ceefa

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 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 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 requested a review from bmeck March 27, 2017 02:59

addaleax reviewed Mar 27, 2017

View reviewed changes

TimothyGu reviewed Mar 27, 2017

View reviewed changes

Address feedback

81236ed

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

addaleax approved these changes Mar 27, 2017

View reviewed changes

jasnell closed this Mar 30, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas added the backport-to-v7.x label Apr 10, 2017

TimothyGu mentioned this pull request Apr 19, 2017

(v7.x backport) url: updates to the WHATWG URL parser #12507

Merged

4 tasks

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 added backported-to-v7.x and removed backport-requested-v7.x labels Apr 27, 2017


		_process.setupRawDebug();

		NativeModule.require('internal/url');

Uh oh!

Conversation

jasnell commented Mar 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 27, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 27, 2017

Uh oh!

jasnell commented Mar 30, 2017

Uh oh!

italoacasas commented Apr 10, 2017

Uh oh!

TimothyGu commented Apr 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants