Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

Still have not given conclusive examples of in-the-wild sites that will break by introducing properties named global or self or window #16

Closed
domenic opened this issue May 21, 2016 · 18 comments

Comments

@domenic
Copy link
Member

domenic commented May 21, 2016

I'm still not in favor of creating an entire new top-level namespace, and a fourth way of referring to the global, until this can be conclusively shown to be web-incompatible.

@ljharb
Copy link
Member

ljharb commented May 21, 2016

What would constitute "conclusive" to you?

@ljharb
Copy link
Member

ljharb commented May 21, 2016

(fwiw, the top-level namespace System is already guaranteed to eventually exist, since it's planned for the reflective module API)

@domenic
Copy link
Member Author

domenic commented May 21, 2016

What would constitute "conclusive" to you?

An example of code found by, say, scraping the Alex top 5K, where adding a property named "global" or "self" or "window" will cause web-visible breakage.

(fwiw, the top-level namespace System is already guaranteed to eventually exist, since it's planned for the reflective module API)

The reflective module API is not guaranteed to exist in its current form; this is not at all a good argument.

@domenic
Copy link
Member Author

domenic commented May 21, 2016

Basically, the bar of proof here has to be as high as the one that prevented us from shipping Array.prototype.values for so long.

@ljharb
Copy link
Member

ljharb commented May 21, 2016

That's why I chose System, and it seemed to be a good enough argument for the committee as a whole. I'm open to other suggestions for a namespace.

Wouldn't sites in the Alexa top 5K be evangelizeable, and thus not apply? If any site breaks, then that imo constitutes "web visible breakage".

As for Array#values, Safari and Edge have shipped it for awhile now, and in the case of Firefox/Chrome, it shipped, and was then held back, due to breakage. I'm not sure that situation applies the same as this one, where we can choose between "maybe won't break anything" versus "definitely can not possibly break anything".

@domenic
Copy link
Member Author

domenic commented May 21, 2016

The situation is the same: we (or in this case I) want to ship a nicely-named something that fits with existing practices. The alternative is something ugly and verbose (Array.prototype.values2, System.global). We should try our best to do the nicely-named, fits with existing practices version, before falling back on the ugly and verbose version. If it's proven web-incompatible---i.e., it's even more web-incompatible than Array.prototype.values was, where we were able to evangelize---then we should fall back to the ugly and verbose proposal.

@ljharb
Copy link
Member

ljharb commented May 21, 2016

I believe @erights had some SES-ish objections to it being a top-level property - hopefully he can clarify, since that might make this discussion moot.

In the meantime, I'll keep trawling code for examples that meet your desired bar.

@domenic
Copy link
Member Author

domenic commented May 21, 2016

@erights has a vision of namespacing "capability-granting" objects inside System, but it's not one that the committee as a whole shares. Existing capability-granting objects are all over the place on the web platform, mostly on window, but there's also Math.random and Date.now, which are not namespaced in System. That is not a sufficient argument.

@domenic
Copy link
Member Author

domenic commented May 21, 2016

In the end, I think we'd be better served by trying to ship window or self or global in some major browsers, and seeing if they're web-compatible, falling back to the ugly-and-verbose version if that fails.

In fact, self is already shipping and has proven web-compatible, so we should probably just go with that.

@ljharb
Copy link
Member

ljharb commented May 21, 2016

self isn't node-compatible, however, and that matters too. In addition, there's the confusion caused by the var self = this pattern.

Based on the last committee consensus, only global and System.global seemed to be in the running. self immo is a much uglier option than System.global, per your earlier "try our best" motive.

@domenic
Copy link
Member Author

domenic commented May 21, 2016

self isn't node-compatible, however, and that matters too

Node accepts breaking changes in each upgrade. For example, the change to the serialization of regexps caused some node apps to break. Similarly the change to TypedArrays to use species. It's important to show that the breakage would be widespread, e.g. use Node's cigtm mechanism on a custom build of V8 that has a global named self to show breakage in the ecosystem.

In addition, there's the confusion caused by the var self = this pattern.

This confusion is already present. Formalizing what already exists does not hurt.

Based on the last committee consensus, only global and System.global seemed to be in the running.

That was not my impression. But I'm happy to try for global; I'd love to have that be the proposal and try shipping it.

@ljharb
Copy link
Member

ljharb commented May 21, 2016

(Linking to #11)

@ljharb ljharb mentioned this issue May 22, 2016
31 tasks
@ljharb
Copy link
Member

ljharb commented May 25, 2016

Some examples of using self breaking non-browser code (it's tough to slog through all the "get the global object" examples to find the important ones)

@domenic
Copy link
Member Author

domenic commented May 25, 2016

So let me try to explain what I think the priorities are here, partially based on discussions within the Chrome team. From highest to lowest:

  1. Do not introduce a new namespace object.
  2. Add a feature to allow access to the global object no matter the environment
  3. Do not expand the list of ways to refer to the global beyond the current cross-environment {global, GLOBAL, root, window, self, frames} union.
  4. Do not expand the list of ways to refer to the global in the browser beyond the current {window, self, frames}.
  5. Do not use the word "global" since it is inaccurate; you must in fact return the global this value, not the global (which the ES spec now distinguishes between: the global environment record's [[ObjectRecord]]'s binding object vs. the global environment record's [[GlobalThisValue]])

Given these priorities our preferred solution is self. It is a breaking change for other environments, like many other things that happen when you upgrade your ES engine. root would also be acceptable. global starts to cross beyond the "is this worth it" barrier. System.global is not really acceptable.

@jdalton
Copy link
Member

jdalton commented May 25, 2016

Just a heads up, at the moment Lodash makes the assumption that if there's free variable self that it's a browser/worker environment and we export to that along with whatever exports are required. If self was introduced in Node it would cause a global leak.

  // Expose Lodash on the free variable `self` when available so it's globally
  // accessible, even when bundled with Browserify, Webpack, etc. This prevents
  // errors in cases where Lodash is loaded by a script tag in the presence of
  // an AMD loader. See http://requirejs.org/docs/errors.html#mismatch for more
  // details. Use `_.noConflict` to remove Lodash from the global object.
  (freeSelf || {})._ = _;

  // Some AMD build optimizers like r.js check for condition patterns like the following:
  if (typeof define == 'function' && typeof define.amd == 'object' && define.amd) {
    // Define as an anonymous module so, through path mapping, it can be
    // referenced as the "underscore" module.
    define(function() {
      return _;
    });
  }
  // Check for `exports` after `define` in case a build optimizer adds an `exports` object.
  else if (freeModule) {
    // Export for Node.js.
    (freeModule.exports = _)._ = _;
    // Export for CommonJS support.
    freeExports._ = _;
  }
  else {
    // Export to the global object.
    root._ = _;
  }

@domenic
Copy link
Member Author

domenic commented May 25, 2016

Yep, it would be a breaking change, like many things are in each new Node version.

@caridy
Copy link

caridy commented May 26, 2016

thoughts:

  • From the object capabilities point of view, and the work that we are doing with realms, it will be simpler if we have a clear separation between "intrinsics", "standard libs", and capability-granting objects. This will help to minimize the cost of creating or spawning a realm, and will help drastically with the cognitive load: It is not the same saying, "anything under System.* will give you some form of authority", than saying: "a, b, and c will...", and as for Math.random and Date.now, we have decided to keep them intact, just like SES does it.
  • Aside from NaN and Infinity, I don't see any global in 262 that is not a primitive value (null and undefined). That is probably not a big deal, but something to keep in mind, proliferation of them could be problematic.
  • We know we will need something to allocate the default structure to import a module, whether that's the current loader, or not, it is irrelevant. System seems like a good place, and it is definitely very subjective. I might be bias here since I'm the editor of the loader spec.
  • We know any global name we choose will have issues: conflicts, shadowing, etc. it is like naming a library these days, it is pretty hard. System seems like a good filter to prevent those issues.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2016

We've decided to rename this to global, so this issue isn't relevant anymore.

@ljharb ljharb closed this as completed Sep 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants