lib: make String(global) === '[object global]'

[addaleax]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

lib

Description of change

This inadvertently changed to [object Object] with the V8 upgrade in 8a24728...96933df. Use Symbol.toStringTag to undo this particular change.

Fixes: #9274

CI: https://ci.nodejs.org/job/node-test-commit/5899/

[cjihrig]

Code changes LGTM. I'm on the fence as to whether or not we should do this though.

[bnoordhuis]

Changes LGTM but I'm on the same fence as @cjihrig.

[fhinkel]

I rewrote Object.prototype.toString. TC39 spec doesn't mention special handling for global, see https://tc39.github.io/ecma262/#sec-object.prototype.tostring.
Are we sure we want different behavior in Node.js?

[addaleax]

Yeah, these are good points. I am in favour of this because a) it comes at a very small cost and b) changing it in the first place broke real-world code (and just in case it matters, I have written code like the one referenced in the issue, too).

[fhinkel]

But setting Symbol.toStringTag is also a breaking change. Probably used a lot less than checking for [object global] but still observable.

[addaleax]

Probably used a lot less than checking for [object global] but still observable.

Right – that “a lot less” part is kind of the point here. ;) I doubt anyone actually relies on the value of global[Symbol.toStringTag], especially given that Symbol.toStringTag support in Node is still relatively new.

[jasnell]

This LGTM. I'm also not entirely sure we should do this but given that it brings the output back to what we have prior to v7 it should be fine for now. We just may want to make a note to revisit this later.

[dnalborczyk]

@jasnell v7 is semver major. why is this "breaking change" a problem if you just add it to the changelog?

[addaleax]

why is this "breaking change" a problem if you just add it to the changelog?

Right, it is not forbidden by semver or anything. To me, it’s more of a “don’t break people’s code without a proper reason” kind of thing, that’s all.

[jasnell]

@dnalborczyk ... i'm sorry, I'm not quite following what you're saying here.

[dnalborczyk]

@addaleax makes sense.

Out of curiosity, when this https://github.com/tc39/proposal-global is coming down the pipe (stage 3) as of now, could that have (eventually) any effect on the above, since it seems to be suggesting 'global' in the proposal?

[addaleax]

Out of curiosity, when this https://github.com/tc39/proposal-global is coming down the pipe (stage 3) as of now, could that have (eventually) any effect on the above, since it seems to be using 'global' in the proposal?

I don’t think so, at least not by looking at the spec changes in there. Maybe @ljharb has an opinion on this issue, and on whether specifying global[Symbol.toStringTag] should be part of the standardization too (although it seems a bit late for that?).

[ljharb]

No, specifying a custom toStringTag is intentionally not part of the proposal - the global object is supposed to be seen as a normal object. It's not required, however, which is why browsers report [object Window].

If you want to specify this, absolutely do it with Symbol.toStringTag and not as magic special behavior - magic behavior is something we're aggressively trying to remove from the spec, and in this case that's the sole reason toStringTagexists.

I would discourage doing this, however, because then people will start testing for the global object with this string, which is something that any object can forge by setting their own toStringTag.

[dnalborczyk]

@jasnell sorry, it might have come over the wrong way. Not a native speaker here. ;) It was merely a question: "... if one (not particularly you) could just mentioning it in the changelog and/or docs?".

[Fishrock123]

The v7 output seems expected to me. I'm actually very surprised that previously it came out as [object global].

I'd like to wait from the issue thread to know who this breaks and how widespread it is.

[mscdex]

I think I'm -1 on this. The change happened in a new major (non-LTS) version of node and anyone performing this kind of check should be using something better (like my suggested process.versions.node existence and type check).

If this v8 change instead happened on the v4.x or v6.x branches, then I could possibly see patching the string.

[addaleax]

anyone performing this kind of check should be using something better (like my suggested process.versions.node check).

Just to be clear, I don’t think anybody disagrees with that. But as mentioned above and in the issue, this is not only used in checking for node.

I'd like to wait from the issue thread to know who this breaks and how widespread it is.

Yeah, I guess I’ll try to run a @ChALkeR-style npm search for [object global] or something.

[mscdex]

But as mentioned above and in the issue, this is not only used in checking for node.

I re-read this PR's comments and those in the linked issue but could not find any such other use cases?

[addaleax]

I’m basically referring to #9274 (comment) (i.e. distinguishing the global object from other objects). I’m not saying it’s a good idea to do that either, but I know for a fact that it happens in real-world code.

[mscdex]

@addaleax I'm curious, why would such a global object comparison be necessary?

[addaleax]

@mscdex One public example I know of is this, where the code basically case-switches over the values of Object.prototype.toString (this is not the one I have in mind, because it actually does not handle global – still, you get the general idea when you look at it). But I know of non-public code that basically does the same kind of thing.

[ljharb]

I'd suggest using Object.defineProperty here, so that you can make it nonenumerable, and preferably nonwritable as well.

[addaleax]

@ljharb thanks for the pointer, updated with that

[ljharb]

you'll need to explicitly specify configurable: true here, or else it will default to false (and generally it's better to leave things configurable so polyfills can fix them in the future if needed)

[addaleax]

@ljharb thanks for pointing that out too, updated!

[ljharb]

(Although I don't get a vote) I'm a strong -1 on including this change - I think it should stay as it is in v7.

That said, if this change is made, the code looks ideal.

[addaleax]

Although I don't get a vote

You do get a voice, and I get the overall impression that most people agree with you. ;)

[jasnell]

@addaleax ... what do you want to do with this? given the -1's, I'm inclined to think we should close it without further review. If this is something you feel we need, however, perhaps label it ctc-review to escalate it?

[sam-github]

I think @hashseed made a good case in #9274 (comment) and #9274 (comment) that setting the tag to global is the right thing. In particular, note that its what the Chromium team did with d8, their CLI javascript interpreter.

[addaleax]

I still think this is the right thing to do, if only because it was basically an unintended breaking change in v7 and I would say that breaking changes should always be conscious decisions, when possible. But tbh, other people’s opinions on this seem far stronger than mine…

So, yeah, if anybody wants to put their foot down and close this, that’s okay.

/cc @nodejs/ctc

[fhinkel]

LGTM. Is this good to merge or are there strong -1s?

[evanlucas]

I +1 on this. I agree with @addaleax that breaking changes should be conscious decisions.

[ljharb]

Because it was unintended means that the discussion should be had - but that doesn't automatically mean reverting it is the right decision.

If this had been noticed prior to the v7 release, I'd still have advocated sticking with "[object Object]" because there needs to be fewer magic things in a JS env, not more.

[addaleax]

@ljharb Yeah, no doubt you’re making a good case for why it should have been [object Object] from the beginning.

Because it was unintended means that the discussion should be had - but that doesn't automatically mean reverting it is the right decision.

Right – in this case it’s just that I think reverting to the old behaviour comes at a low cost on Node’s side, and that doing so is in line with valuing stability over progress. If this PR lands, reverting it again in the future for v8 or v9 sounds like a good idea to me, yes; Until then we should be able to remove a lot of the (admittedly already very low) ecosystem usage of [object global].

I’m labelling this for the @nodejs/ctc agenda because I think this could benefit from a bit of quick synchronous discussion (and because its more important to me to have this resolved than the specific outcome).

[ofrobots]

I'm +1 on this PR as well for v7.x.

[mscdex]

So how are module authors supposed to know to change this come v8/v9? There's no deprecation warning or anything like that, or are we supposed to rely on "word of mouth?" Otherwise we will run into the same problem in April or whenever...

[rvagg]

+0 from me, basically abstaining. While I can't conceive of any situation where I might want to do this kind of detection I accept that we have such a broad community of users that use-cases may exist for some of them so I find it hard to be absolute about this not being necessary. I accept the argument that breaking changes should be intentional but it doesn't seem to fully apply here I think since it's a V8 upgrade, which itself is a breaking change and we've treated it as such in the past unless we've been able to patch things. Mind you, if the Chromium folks consider this a worthwhile thing to do in d8 then perhaps it should be considered something of a mistake?

(How's that for some artisanal fence sitting?)

[bnoordhuis]

Mind you, if the Chromium folks consider this a worthwhile thing to do in d8 then perhaps it should be considered something of a mistake?

I suspect that was mostly a matter of convenience. There are a number of tests that check the result of evaluating this in the debugger (edit: not just debugger tests). It was probably quicker and less invasive to patch d8 than to chase down all tests.

[addaleax]

So how are module authors supposed to know to change this come v8/v9? There's no deprecation warning or anything like that, or are we supposed to rely on "word of mouth?"

I don’t think I have better suggestions than making PRs for occurrences found in npm packages and the fact that this isn’t a huge problem as it is.

[hashseed]

Please do not use d8 code as authority for anything. It's little more than a test bed for V8 test cases. However, consider that the window object in Chrome has the toStringTag "Window". So this is defintely the correct way to do this, if we want to keep the old behavior.

[mhdawson]

I'm +1 on this PR for v7.x based on the goal of least surprise for users.

[addaleax]

Since the vote in this week’s CTC meeting resulted in accepting this PR in its current state with the desire of getting it into the second v7.x release, I’m landing this.

(CI is green except for 2 unrelated smartos failures)

[addaleax]

Landed in 0fb21df