global breaks flickr.com #20

Open
evilpie opened this Issue Dec 28, 2016 · 27 comments

Projects

None yet
@evilpie
evilpie commented Dec 28, 2016 edited

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

@ljharb
Member
ljharb commented Dec 28, 2016

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
ziyunfei commented Dec 29, 2016 edited

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

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

image

@ljharb
Member
ljharb commented Dec 29, 2016

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
Member
ljharb commented Dec 29, 2016

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
jswalden commented Dec 29, 2016 edited

Unless I'm misreading the code (entirely possible), #20 (comment) looks wrong to me. Flickr's moment.js code is identical to that on trunk, and that's been on trunk since moment/moment@1601cb1 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.

@evilpie evilpie changed the title from global is not webcompatible to global breaks flickr.com Dec 29, 2016
@ljharb
Member
ljharb commented Dec 30, 2016

@jswalden thanks, I think your conclusion is correct.

@ljharb
Member
ljharb commented Dec 31, 2016

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.

@ljharb ljharb referenced this issue in nodejs/node Jan 6, 2017
Open

src: make `global` non-enumerable #10405

3 of 4 tasks complete
@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
evilpie commented Jan 10, 2017

I should mention we removed global from Nightly again.

@chicoxyzzy chicoxyzzy referenced this issue in kangax/compat-table Jan 10, 2017
Merged

Remove global from FF Nightly #1004

@nt1m
nt1m commented Jan 10, 2017

@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
evilpie commented Jan 11, 2017

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
Member
ljharb commented Jan 11, 2017

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
rjgotten commented Jan 11, 2017 edited

@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
targos commented Jan 11, 2017 edited

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
Member
ljharb commented Jan 11, 2017 edited

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
Member

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

@leobalter
Member

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
jakub-g commented Jan 11, 2017

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

@ljharb
Member
ljharb commented Jan 11, 2017

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

@ljharb
Member
ljharb commented Jan 11, 2017

@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
Member
ljharb commented Jan 11, 2017

@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
Member
ljharb commented Jan 12, 2017

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

@inexorabletash inexorabletash referenced this issue in inexorabletash/polyfill Jan 14, 2017
Open

Yoink global global #118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment