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 support? #2

Closed
stevenvachon opened this issue Apr 19, 2017 · 26 comments
Closed

URL support? #2

stevenvachon opened this issue Apr 19, 2017 · 26 comments

Comments

@stevenvachon
Copy link

const {URL} = require("url");
const url = "http://domain/";

expect( new URL(url) ).to.deep.match( new URL(url) );
expect({ key:new URL(url) }).to.deep.match({ key:new URL(url) });  // throws
@JamesMGreene
Copy link
Owner

What module is "url" here? Because if it is the Node.js core module "url", then it doesn't even have a constructor... which would indeed throw an Error.

@stevenvachon
Copy link
Author

"url" is Node's url, which in v7.x has an implementation of URL:

const {URL} = require("url") is ES2015 destructuring for const URL = require("url").URL.

@JamesMGreene
Copy link
Owner

Ah, yeah, sorry, I missed the destructuring. I'll give it a quick look over lunch if I can but I see no reason why it shouldn't just work unless the experimental implementation of the URL constructor in Node 7.x is buggy.

@stevenvachon
Copy link
Author

It shouldn't be buggy, especially when we're so close to 8.x which will drop the "experimental".

@JamesMGreene
Copy link
Owner

JamesMGreene commented Apr 19, 2017

So I am able to reproduce this, though I still don't fully understand why. The primary issue here, though, is the URL objects apparently do not have any keys (owned properties), which seems very strange semantically.

For example, myUrl.hostname is not an owned property of myUrl... seems like it should be!

@stevenvachon
Copy link
Author

stevenvachon commented Apr 19, 2017

They're property descriptors, accessed via

Object.getOwnPropertyDescriptor(URL.prototype, "hostname").get

@stevenvachon
Copy link
Author

stevenvachon commented Apr 19, 2017

I've written isurl which will come in handy if you need to special case something.

@JamesMGreene
Copy link
Owner

Why is isurl marked as requiring Node >= 4.x? That would make me hesitate to use it.

@JamesMGreene
Copy link
Owner

FYI, I opened an issue for this on lodash's issue tracker to see if they would consider supporting these URL objects internally. If not, then I can consider adding them as a special case here. If they weren't potentially so prolific in the future, I'd definitely vote against it as they seem ridiculously un-semantic (utilizing inherited property descriptor getter functions rather than normal owned properties (or owned property getters)).

@stevenvachon
Copy link
Author

stevenvachon commented Apr 24, 2017

Why is isurl marked as requiring Node >= 4.x? That would make me hesitate to use it.

Because anything lower has reached end-of-life: https://github.com/nodejs/LTS#lts-schedule1

@JamesMGreene
Copy link
Owner

So, basically I think I would just add something like the following (only more optimized, obviously) around index.js#L27:

if ( Object.prototype.toString.call( actual ) === '[object URL]' ) {
  actual = require('url').parse( actual.href );
}
if ( Object.prototype.toString.call( expected ) === '[object URL]' ) {
  expected = require('url').parse( actual.href );
}

@stevenvachon
Copy link
Author

stevenvachon commented Apr 26, 2017

That only supports Node v7+. Unless using isurl is part of "more optimized".

@JamesMGreene
Copy link
Owner

Isn't this only an issue for URL objects that come out of Node v7+?

@JamesMGreene
Copy link
Owner

JamesMGreene commented Apr 26, 2017

I suppose it doesn't handle a URL object deeper down within an object, though. I'd have to use something like traverse for that.

@stevenvachon
Copy link
Author

stevenvachon commented Apr 26, 2017

Isn't this only an issue for URL objects that come out of Node v7+?

Yes, you're right. I just tested deep-nested instances of whatwg-url and it passes fine.

@JamesMGreene
Copy link
Owner

It works with whatwg-url because that module produces sane (semantic) URL objects that have owned properties, unlike the real WHATWG URLs in Node 7.x and the browser that use weird getters instead.

@stevenvachon
Copy link
Author

stevenvachon commented Apr 27, 2017

Will require('url').parse work in a web browser? Also, URL exists for a reason -- url.parse does not parse the same. Comparing the href properties for equality should be sufficient, anyway.

@JamesMGreene
Copy link
Owner

JamesMGreene commented Apr 27, 2017

Will require('url').parse work in a web browser?

If you were using something like Browserify/Webpack, then I assume they will add appropriate support to make it work in the future if it is not already there today. If you are not using those tools, then I'd imagine the answer is no.

That said, this module (chai-deep-match) currently doesn't work in the browser either.

Also, URL exists for a reason -- url.parse does not parse the same.

Oh, bummer. Hmm....

@JamesMGreene
Copy link
Owner

JamesMGreene commented Apr 27, 2017

@stevenvachon: To the best of your knowledge, is it possible for 2 URL objects to...

  • Be considered different, while having matching href values?
  • Be considered matching, while having different href values?

@stevenvachon
Copy link
Author

Nope to both.

@stevenvachon
Copy link
Author

stevenvachon commented Apr 28, 2017

I just realized that my answer was not necessarily correct. For an assertion library, I would say that two non-matching href properties are different URLs. If someone needs deeper context than that, they can use url-relation to catch things like:

url1 = 'http://host/?p1&p2'
url2 = 'http://host/?p2&p1'

Some servers may not be configured correctly (according to specification and common best practices) by treating those as two URLs different, when they should technically be the same.

@stevenvachon
Copy link
Author

@JamesMGreene would Object.is() work?

@stevenvachon
Copy link
Author

@JamesMGreene any progress on this?

@JamesMGreene
Copy link
Owner

@stevenvachon Just to be clear: are you only expecting the URLs to be considered matching if they are full matches vs subsets? For example, would you want the following to be considered a match?

var url1 = new URL('https://github.com/blog?foo=bar&baz=wat');
var url2 = new URL('https://github.com/blog?baz=wat');

// Should this pass on partial match or fail because they are effectively different URLs?
expect( url1 ).to.deep.match( url2 );

@stevenvachon
Copy link
Author

stevenvachon commented Jun 11, 2018

Full match, but a subset might be useful as an additional method somehow.

You might be able to use my--currently incomplete--url-relation for a pre-release. I'll be fixing its edge cases soon.

@JamesMGreene
Copy link
Owner

Published as v1.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants