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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 25, 2016

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/

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 25, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Oct 25, 2016

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

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@fhinkel
Copy link
Member

fhinkel commented Oct 25, 2016

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
Copy link
Member Author

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
Copy link
Member

fhinkel commented Oct 25, 2016

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

@addaleax
Copy link
Member Author

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.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

dnalborczyk commented Oct 25, 2016

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

@addaleax
Copy link
Member Author

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
Copy link
Member

jasnell commented Oct 25, 2016

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

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Oct 25, 2016

@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
Copy link
Member Author

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
Copy link
Member

ljharb commented Oct 25, 2016

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 toStringTag exists.

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
Copy link
Contributor

dnalborczyk commented Oct 25, 2016

@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
Copy link
Contributor

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
Copy link
Contributor

mscdex commented Oct 25, 2016

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
Copy link
Member Author

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
Copy link
Contributor

mscdex commented Oct 25, 2016

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
Copy link
Member Author

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
Copy link
Contributor

mscdex commented Oct 25, 2016

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

@addaleax
Copy link
Member Author

@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.

@@ -188,6 +188,7 @@
}

function setupGlobalVariables() {
global[Symbol.toStringTag] = 'global';
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb thanks for the pointer, updated with that

Object.defineProperty(global, Symbol.toStringTag, {
value: 'global',
writable: false,
enumerable: false
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb thanks for pointing that out too, updated!

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

Fixes: nodejs#9274
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

(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
Copy link
Member Author

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
Copy link
Member

jasnell commented Oct 28, 2016

@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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

fhinkel commented Nov 1, 2016

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

@evanlucas
Copy link
Contributor

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

@ljharb
Copy link
Member

ljharb commented Nov 1, 2016

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
Copy link
Member Author

addaleax commented Nov 1, 2016

@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
Copy link
Contributor

ofrobots commented Nov 1, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Nov 1, 2016

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
Copy link
Member

rvagg commented Nov 2, 2016

+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
Copy link
Member

bnoordhuis commented Nov 2, 2016

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
Copy link
Member Author

addaleax commented Nov 2, 2016

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
Copy link
Member

hashseed commented Nov 2, 2016

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
Copy link
Member

mhdawson commented Nov 2, 2016

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

@addaleax
Copy link
Member Author

addaleax commented Nov 2, 2016

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
Copy link
Member Author

addaleax commented Nov 2, 2016

Landed in 0fb21df

@addaleax addaleax closed this Nov 2, 2016
@addaleax addaleax deleted the global-to-string-tag branch November 2, 2016 22:27
addaleax added a commit that referenced this pull request Nov 2, 2016
This inadvertently changed to `[object Object]` with the V8 upgrade
in 8a24728...96933df. Use `Symbol.toStringTag` to undo this
particular change.

Fixes: #9274
PR-URL: #9279
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
addaleax added a commit that referenced this pull request Nov 2, 2016
This inadvertently changed to `[object Object]` with the V8 upgrade
in 8a24728...96933df. Use `Symbol.toStringTag` to undo this
particular change.

Fixes: #9274
PR-URL: #9279
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.