Skip to content

url: use symbol properties correctly in URL class

Rodrigo Muino Tomonari requested to merge github/fork/TimothyGu/url-props into master

This PR has two commits, both concerning the usage of symbol properties.

The first is to avoid using a getter for @@toStringTag. It does mean that parts of #10562 are reverted. This is done for multiple reasons:

  1. Web IDL says "the object must, at the time it is created, have a property whose name is the @@toStringTag symbol and whose value is the specified string." This implies that the object itself must possess the property, not its Prototype.

  2. From the same quote above, the spec says the property must have a value, (not a getter). Elsewhere in the spec, the distinction between getter, setter, and value of a property is always well-defined, and there is no reason to assume otherwise for this instance.

  3. Ambiguity of behavior when something like this happens:

    Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag).get();

    Web IDL is usually fairly strict with this values (see operations/methods, getters and setters). In fact, all of the following will result in an error in the browser:

    Object.getOwnPropertyDescriptor(URL.prototype, 'href').get();
Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call(null);
Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call(window);
Object.getOwnPropertyDescriptor(URL.prototype, 'href').get.call({});

    However, because of the second concern above, no getter for @@toStringTag is defined in the spec, and therefore no algorithms exist for dealing with this situation: whether the getter should return 'URL', 'URLPrototype', '', undefined, or throw an error cannot be reasonably decided. Currently, even Object.getOwnPropertyDescriptor(URL.prototype, Symbol.toStringTag).get.call(null) returns 'URLPrototype', which makes little sense.

This change does also need to be applied to URLSearchParams class. I will do so in a subsequent PR.

The second is arguably less controversial. The issue is that an undocumented but public inspect() exists on URL.prototype, even though it is not part of the URL Standard. It was first raised by @watilde in https://github.com/nodejs/node/pull/10857#issuecomment-273265162. This PR fixes this issue by renaming the property to an internal symbol reserved for inspect method, which is already how URLSearchParams' inspection operates. Tests are adjusted accordingly.

/cc @nodejs/url, @watilde

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url-whatwg

Merge request reports