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: introduce native URL class for internal use #11801

Closed
wants to merge 1 commit into from
Closed

url: introduce native URL class for internal use #11801

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 11, 2017

Provides a native URL class that uses the WHATWG URL algorithm.
This is intended primarily to support the ES6 modules implementation work (which will need WHATWG URL processing).

/cc @bmeck

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

url

@jasnell jasnell added dont-land-on-v4.x 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 11, 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 whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 11, 2017
@TimothyGu
Copy link
Member

Can you add some gtests for this new API?

addaleax
addaleax reviewed Mar 11, 2017
View reviewed changes
src/node_url.cc Outdated

inline void SetFlag(struct url_data* url, enum url_flags flag) {
url->flags |= flag;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it’s just me, but I’d probably find it more readable if these methods were just inlined

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm... I guess... I prefer the inlined methods personally but I can change it up

src/node_url.h Outdated
const size_t len,
enum url_parse_state state_override,
struct url_data* url,
struct url_data* base,
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 probably be const struct url_data* base?

src/node_url.h Outdated
Parse(input, len, kUnknownState, &context_, nullptr, false);
}

URL(const char* input, const size_t len, URL* base) {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, const URL* base?

src/node_url.h Outdated

std::string protocol() {
return context_.scheme;
}
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 there’s no reason to not make these methods return const references, i.e. const std::string& protocol() const { … } (except path())?

@jasnell
Copy link
Member Author

jasnell commented Mar 11, 2017

@TimothyGu ... yep, I intend to, just ran out of time on Friday and wanted to get this out so that @bmeck had something he could start working with. Will look at adding tests on Monday.

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2017

@addaleax ... addressed your feedback.

@TimothyGu ... adding a gtest is going to be more involved. I'd like to add that later in a separate PR so that I can get this landed for @bmeck to use.

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2017

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

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2017

CI is 🍏

addaleax
addaleax approved these changes Mar 22, 2017
View reviewed changes
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.

LGTM

(And yeah, this is definitely more readable. Thank you!)

@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2017

@TimothyGu ... once #11956 lands I will be able to get tests added for the native class.

@jasnell
src: add native URL class
5be93fd 
Adds a URL native class for use within the node.js c/c++
code. This is primarily intended to be used by the eventual
ES6 modules implementation but can be used generally wherever
URL parsing within the c/c++ may be necessary.

```c
URL url1("http://example.org");
URL url2("foo", "http://example.org/bar");
URL url3("baz", &url2);
```

While we're at it, reduce reliance on macros to simplify impl.
jasnell added a commit that referenced this pull request Mar 22, 2017
@jasnell
src: add native URL class
e26b6c6 
Adds a URL native class for use within the node.js c/c++
code. This is primarily intended to be used by the eventual
ES6 modules implementation but can be used generally wherever
URL parsing within the c/c++ may be necessary.

```c
URL url1("http://example.org");
URL url2("foo", "http://example.org/bar");
URL url3("baz", &url2);
```

While we're at it, reduce reliance on macros to simplify impl.

PR-URL: #11801
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Mar 22, 2017

Landed in e26b6c6

@jasnell jasnell closed this Mar 22, 2017
@jasnell jasnell mentioned this pull request Mar 25, 2017
Closed
3 tasks
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
@jasnell @MylesBorins
src: add native URL class
d0c2d67 
Adds a URL native class for use within the node.js c/c++
code. This is primarily intended to be used by the eventual
ES6 modules implementation but can be used generally wherever
URL parsing within the c/c++ may be necessary.

```c
URL url1("http://example.org");
URL url2("foo", "http://example.org/bar");
URL url3("baz", &url2);
```

While we're at it, reduce reliance on macros to simplify impl.

PR-URL: #11801
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
Merged
@addaleax addaleax removed the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 28, 2017
MylesBorins added a commit that referenced this pull request Mar 28, 2017
@MylesBorins
2017-03-28, Version 7.8.0 (Current)
f04524e 
Notable changes:

* buffer:
  - do not segfault on out-of-range index (Timothy Gu)
    #11927
* crypto:
  - Fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  * upgrade npm to 4.2.0 (Kat Marchán)
    #11389
  * fix async await desugaring in V8 (Michaël Zasso)
    #12004
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - add native URL class (James M Snell)
    #11801

PR-URL: #12104
MylesBorins added a commit that referenced this pull request Mar 29, 2017
@MylesBorins
2017-03-28, Version 7.8.0 (Current)
9b2dd47 
Notable changes:

* buffer:
  - do not segfault on out-of-range index (Timothy Gu)
    #11927
* crypto:
  - Fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  * upgrade npm to 4.2.0 (Kat Marchán)
    #11389
  * fix async await desugaring in V8 (Michaël Zasso)
    #12004
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - add native URL class (James M Snell)
    #11801

PR-URL: #12104
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-28, Version 7.8.0 (Current)
e0661dd 
    Notable changes:

    * buffer:
      - do not segfault on out-of-range index (Timothy Gu)
        nodejs/node#11927
    * crypto:
      - Fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      * upgrade npm to 4.2.0 (Kat Marchán)
        nodejs/node#11389
      * fix async await desugaring in V8 (Michaël Zasso)
        nodejs/node#12004
    * readline:
      - add option to stop duplicates in history (Danny Nemer)
        nodejs/node#2982
    * src:
      - add native URL class (James M Snell)
        nodejs/node#11801

    PR-URL: nodejs/node#12104

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jasnell @TimothyGu @addaleax @nodejs-github-bot