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: remove Object from global's prototype #5888

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Mar 24, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

Affected core subsystem(s)

lib

Description of change

Removes some strange behavior from having Object in the global object's
prototype chain. No longer introducing global variables coming from
Object.prototype.

These globals have strange warts from existing but we should throw this in CITGM this to see if anything is affected. The only thing I really am suspicious of is toString might be used by some people via type coercion like var_that_refs_global+''.

This would be a breaking change.

Removes some strange behavior from having Object in the global object's
prototype chain. No longer introducing global variables coming from
`Object.prototype`.
@bmeck
Copy link
Member Author

bmeck commented Mar 24, 2016

CC: @trevnorris

@silverwind silverwind added lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 24, 2016
@@ -208,7 +208,6 @@

function setupGlobalVariables() {
global.process = process;
global.global = global;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, we were assigning this again?? Should I be removing this bit in my PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell
Copy link
Member

jasnell commented Mar 24, 2016

Seems reasonable. But let's definitely see what citgm says. /cc @thealphanerd

@Fishrock123
Copy link
Contributor

@bmeck
Copy link
Member Author

bmeck commented Mar 24, 2016

well the lack of a global toString variable seems to make CITGM blow up

@Slayer95
Copy link

Relevant: http://www.ecma-international.org/ecma-262/6.0/index.html#sec-global-object

The value of the [[Prototype]] internal slot of the global object is implementation-dependent.

@bmeck
Copy link
Member Author

bmeck commented Mar 24, 2016

@Slayer95 correct, but breaking existing libraries is a bad sign

@Fishrock123
Copy link
Contributor

lodash: fnToString.call(toString)

Oh my. Yeah I'm not sure this will be something we can do. Looks like Chrome also exposes toString(). (and the prototype?)

@Fishrock123
Copy link
Contributor

Maybe we could expose specific ones like toString()?

@bmeck
Copy link
Member Author

bmeck commented Mar 24, 2016

@Fishrock123 some people are explicitly checking all known globals by variable name, like in readable-stream/test/common.js and they get reference errors when they don't exist.

@bmeck
Copy link
Member Author

bmeck commented Mar 28, 2016

Going to close this after lots of CITGM attempts on my own, too many people using it in the wild.

@bmeck bmeck closed this Mar 28, 2016
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. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants