global breaks flickr.com

[evilpie]

See bug 1325907 . Just the existence of global seems to break flickr.com. This caused by code in the popular moment/moment library.

[ljharb]

The referenced code - https://github.com/moment/moment/blob/94ad539f965689289af54b6493abca0a65ab4ce6/moment.js#L15 - should work identically with global, unless the file is naively concatenated such that it's in strict mode, such that top-level this isn't the global object.

Is that what Flickr is doing? If so, lots of things might be broken by their build process :-/

[ziyunfei]

The moment.js code is wrapped in a YUI module:

YUI.add("moment", function(e, t) {
  alert(this === global) // false 
}, "@VERSION@")

[ljharb]

In that case, moment should not be wrapping YUI, since that would break in node.

I realize this doesn't change the web compatibility issue - but it does highlight the incorrect assumptions that were made when wrapping moment.

@evilpie are there any other reports? I feel like we should be able to evangelize to Yahoo and get them to update Flickr, and similarly, update moment via a PR, which would remove the web compat breakage.

[ziyunfei]

The typeof global checking had already been removed from new versions of moment.js.

[ljharb]

Given that, it sounds like we just need to get Flickr/Yahoo to update the version of moment that it's using, and the problem goes away. @evilpie, if this happens, would we be able to try this again in Firefox?

[jswalden]

Unless I'm misreading the code (entirely possible), https://github.com/tc39/proposal-global/issues/20#issuecomment-269586160 looks wrong to me. Flickr's moment.js code is identical to that on trunk, and that's been on trunk since https://github.com/moment/moment/commit/1601cb1dd7b14277ba8b00cb2ece3ce637923080 two years ago. The seeming difference between

(typeof global !== 'undefined' && (typeof window === 'undefined' || window === global.window)) ? global : this

and

typeof global == "undefined" || typeof window != "undefined" && window !== global.window ? this : global

is purely the result of minifying: applying de Morgan's Laws, removing parentheses that aren't necessary by language precedence order, and trimming strict-equality operators to loose equality when both operands always have the same type.

The issue here is purely that Flickr is embedding moment.js code, that uses this presuming it's the global object, in a context where this is not the global object.

[ljharb]

@jswalden thanks, I think your conclusion is correct.

[ljharb]

Another example has popped up in Firefox: bug 1326032

[littledan]

This is all very sad. Sounds like we can't go with the name global for a long time. I'd be very hesitant to put this in V8 given the evidence already collected from Firefox.

[miketaylr]

Another example has popped up in Firefox: bug 1326032

Also, JIRA, in https://bugzilla.mozilla.org/show_bug.cgi?id=1328218 (due to moment.js).

[evilpie]

I should mention we removed global from Nightly again.

[nt1m]

@evilpie : I agree with https://bugzilla.mozilla.org/show_bug.cgi?id=1317422#c18 Can't we have it for strict contexts? That would limit the amount of pages affected if any

[evilpie]

I don't think so. Consider two scripts included in HTML, they can have different strictness, but they still have the same global object, with the "global" on it.

[ljharb]

We might be able to add it in Modules, but adding it in strict mode wouldn't work - you may forget that a Script can have function sloppy() {} function strict() { 'use strict'; } - global isn't a keyword, so it can't have contextual meaning, and it'd be really weird for a string pragma to bring something into scope.

[rjgotten]

@jswalden Unless I'm misreading the code (entirely possible)

You are misreading the code. Those lines are not at all there in trunk, i.e., in https://github.com/moment/moment/blob/master/moment.js

They're there in version 2.0.9 which was tagged 2015-01-02, but were removed when version 2.10.2 which was tagged 2015-04-09, introduced a UMD wrapper.

So yes; this would be resolved by Yahoo/Flickr/etc. updating their codebase.

[littledan]

@ljharb Modules have the same global object as non-modules. You'd add this in the global lexical tier? That would be a new one. Is there anything else we might want to add this way? Adding it only in modules would give this feature significantly diminished utility as well.

What if we named this 'self'? Have we seen evidence that that would break the Node world?

[targos]

What if we named this 'self'? Have we seen evidence that that would break the Node world?

I can try to add self to Node and see if CITGM breaks.

Edit: CITGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/520/

[ljharb]

I won't be actually pursuing "add it only in Modules", I agree the utility would be greatly reduced.

The proper solution here is either to come up with a web-compatible top-level name that has "global" in it ("self" is a terrible name), or to namespace it under a new or existing web-compatible top-level name.

[michaelficarra]

It looks like we have reason to introduce System.global after all.

[leobalter]

If we want to avoid a namespace my suggestion is to pursue something not perfect but less likely to break web compat as __global__, or even $$global. It hurts my eyes, but it's still a single name binding with a safer approach avoiding namespace.

[jakub-g]

__global__ seems rather safe IMO, and in line with stuff like__proto__. Though indeed it looks ugly.

[ljharb]

@targos it looked like the CIGTM run failed a bunch of the jobs?

[ljharb]

@leobalter System.global is far more elegant than __global__ or $$global, though, and a namespace of any kind allows for us to use "global" untainted by legacy characters.

[kdzwinel]

FYI Safari Technology Preview 21 was just released and it features support for the global property. flickr.com doesn't work, but JIRA (v6.2) seems to be working fine for me.

[ljharb]

@kdzwinel can you comment on the firefox bug related to jira? It should be broken in both places if this is indeed the cause.

[miketaylr]

FYI Safari Technology Preview 21 was just released and it features support for the global property. flickr.com doesn't work, but JIRA (v6.2) seems to be working fine for me.

And backed out: https://bugs.webkit.org/show_bug.cgi?id=166915

[ljharb]

(Turns out it requires a newer Jira version to get the failure)

[andyearnshaw]

Is it feasible to continue with global, but have vendors apply a patch to the offending moment.js scripts? For instance, if they could recognise the script and introduce a scope layer at the top level of the script where global is undefined? From what I understand, moment.js was the only reported incompatibility, so this seems like it may be worth trying to "un-stall" this feature.

I realise there's no precedent for this when introducing a new global property, but I know that vendors have patched some JS scripts before.

[ljharb]

@andyearnshaw browsers have indicated that it's not feasible. (I'm also not aware of browsers ever "patching" code on websites in the way you're indicating, do you have any links?)

[andyearnshaw]

@ljharb no links, just something I'm sure I recall happening before. I'm not sure it happened "in the way I was indicating" and I could be completely wrong. I know for definite that Opera patches Google.com to hide the Chrome ads:

Opera has modified script or content on www.google.co.uk (PATCH-1197, Hide Chrome ad from main Google page). See browser.js for details Opera has modified script or content on www.google.co.uk (PATCH-1223, Hide another Chrome ad from main Google page). See browser.js for details

...but this isn't really the same thing.

[michaelficarra]

I mean, if all of the offending scripts have the same hash, the browser could just replace them with a compatible but fixed version. All browsers already have a mechanism for hashing scripts if they support CSP or SRI. Seems worth suggesting to implementors.

[mgol]

@michaelficarra That won't work for people using bundlers like Webpack or Browserify or if they minify themselves, concatenate themselves etc.

[miketaylr]

@michaelficarra That won't work for people using bundlers like Webpack or Browserify or if they minify themselves, concatenate themselves etc.

That won't work for the simple reason that browsers don't want to be in the business of being a CDN for up to date JS lib versions.

[littledan]

Different browsers will have different policies here, but as a way of rolling out a new feature, I think this is pretty safely a dead end.

[syg]

Did we ever bikeshed new names and agree on the next name for browsers to play whack-a-mole with?

[ljharb]

Not yet. Mainly we need a browser to volunteer to try one, and I can pick from the bikeshed list.

[littledan]

@ljharb I think if you go and pick something, and get agreement from everybody that it'd be a good name if it works, then browsers might be more excited to try it; it can be hard to preemptively volunteer to ship in the absence of an agreed-on proposal.

[ljharb]

Sounds good; I'll get some consensus this week and settle on our next attempt.

[hax]

Any update?

[ljharb]

No update yet.

[JakeChampion]

How about we use System.global?

[littledan]

We don't have a System object yet. System.global has been proposed in TC39, but IIRC some committee members objected that global is a little to small to justify System itself. Plus, such a long name might not be very attractive to use.

[littledan]

Due to this issue, the global proposal does not seem like it will proceed to Stage 4 in this form. Looking at this repository, I'm having trouble finding any indication outside of this bug that the name global is not going to be used going forward. I think it'd be best if the README.md and spec had a note mentioning this fact, and replacing global with a clearly unusable placeholder. @phoddie mentioned to me that Moddable has implemented this proposal, unaware of the specification status (even if it might be fine for them to ship this long term, like Node does); seems like we could prevent the same from happening for future implementers.

[ljharb]

That sounds like a good idea. I'll update the readme and spec with a placeholder.

Moddable is a rare event; a new engine that wasn't present during the committee meetings for this proposal :-)

[littledan]

Actually I think there are a lot of people who look at proposal repositories who can't come to meetings. For one, engines have multiple people working on them; there are also popular polyfills without committee representation. Proposal repositories should explain themselves well for all of these audiences.

I'd recommend writing these warnings in bigger letters and actually scratching out mentions of global. People could gloss over these notes as they are written now. Also, I wouldn't mind if this were somehow noted in the proposals repository that it is not ready.

[aciccarello]

The naming section of the readme should also be updated. It currently states

Further research has determined that using global will not break existing code.

[ljharb]

All of the 4 browsers have existing bugs filed that say to hold off implementing; there's really only two popular polyfills i know of, one's mine, and the other's author follows this repo closely.

This seems like an overabundance of caution to me; non-browsers always have to be aware of the web compatibility risk in stage 3 (perhaps the process document needs to indicate that more clearly?).

[littledan]

Thanks, I appreciate the follow-on change. My experience so far with TC39 is that there are lots of people looking at these proposals, different people are going to look at different things, and it'll be helpful to some people who end up skimming your explainer here that the current status is more clear (it's not like I do a perfect job of this myself).

[sag1v]

Do we have to choose a name from the already existing list (global, window, this, frame...) ? How about root?

[ljharb]

No, we don’t have to choose one from an existing list.