1440284 - Change this.EXPORTED_SYMBOLS back to var EXPORTED_SYMBOLS in JS modules

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Florian Quèze [:florian]]

There was a mass change from 'var EXPORTED_SYMBOLS = ' to 'this.EXPORTED_SYMBOLS = ' in bug 798491 to accommodate a hack that was put in place for B2G. We can now revert to the previous style that looked nicer.

Comment 1

[Florian Quèze [:florian]]

Attachment: script-generated patch

This patch is generated using the script at
https://bitbucket.org/fqueze/xpcshell-rewrites/commits/76948c300b1a2237dcdb7072db237a885828aed0

For instructions at the top level, it does:

this.foo = -> var foo =

this.foo = function foo( -> function foo(

this.foo = foo -> just remove the instruction.

Comment 2

[Florian Quèze [:florian]]

Attachment: hand cleanup patch

This cleans up by hand the few cases where the script made a mess, and reverts some changes that caused test failures.

Typical reasons for reverting changes include:
class Foo {...
var EXPORTED_SYMBOLS = ["foo"]
This doesn't work without the 'this.Foo = Foo;' line. Same for 'const Foo'.
Removing the 'this.Foo = Foo;' line works well for the 'function Foo(' and 'var Foo = ' cases.

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67e9fba896e5cb09519203a428caba881782eaba

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8953048 [details] [diff] [review]
hand cleanup patch

Review of attachment 8953048 [details] [diff] [review]:
-----------------------------------------------------------------

Please get somebody else to look at the two .eslintrc.js changes as I don't understand them. I don't see how they are related to the changes in the patch.

::: intl/l10n/L10nRegistry.jsm
@@ +75,5 @@
>   * and will produce a new set of permutations placing the language pack provided resources
>   * at the top.
>   */
>  
> +var L10nRegistry = {

Why did you de-const this instead of adding back the this. = line?

::: toolkit/modules/GMPInstallManager.jsm
@@ -3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -var EXPORTED_SYMBOLS = [];

Oh wow, there's a second EXPORTED_SYMBOLS. Weird...

Comment 4

[Andrew McCreight [:mccr8]]

Florian explained over IRC to me that the eslint changes are just making us use the default global behavior now, which is better, so that's fine.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8953046 [details] [diff] [review]
script-generated patch

Review of attachment 8953046 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/jsat/AccessFu.jsm
@@ +5,5 @@
>  /* exported AccessFu */
>  
>  "use strict";
>  
> +var EXPORTED_SYMBOLS = ["AccessFu"]; // jshint ignore:line

Can you get rid of these jshints (present in a number of files) now? It feels out of scope for this bug, but at least it would be nice if you could file a bug on it.

::: browser/extensions/shield-recipe-client/vendor/PropTypes.js
@@ -1,1 @@
> -/* eslint-disable */this.PropTypes=function(a){function b(d){if(c[d])return c[d].exports;var e=c[d]={i:d,l:!1,exports:{}};return a[d].call(e.exports,e,e.exports,b),e.l=!0,e.exports}var c={};return b.m=a,b.c=c,b.d=function(a,c,d){b.o(a,c)||Object.defineProperty(a,c,{configurable:!1,enumerable:!0,get:d})},b.n=function(a){var c=a&&a.__esModule?function(){return a['default']}:function(){return a};return b.d(c,'a',c),c},b.o=function(a,b){return Object.prototype.hasOwnProperty.call(a,b)},b.p='',b(b.s=100)}({0:function(a){'use strict';var g=function(){};!1,a.exports=function(h,i,j,a,b,c,d,e){if(g(i),!h){var f;if(void 0===i)f=new Error('Minified exception occurred; use the non-minified dev environment for the full error message and additional helpful warnings.');else{var k=[j,a,b,c,d,e],l=0;f=new Error(i.replace(/%s/g,function(){return k[l++]})),f.name='Invariant Violation'}throw f.framesToPop=1,f}}},100:function(a,b,c){a.exports=c(101)()},101:function(a,b,c){'use strict';var d=c(5),e=c(0),f=c(19);a.exports=function(){function a(a,b,c,d,g,h){h===f||e(!1,'Calling PropTypes validators directly is not supported by the `prop-types` package. Use PropTypes.checkPropTypes() to call them. Read more at http://fb.me/use-check-prop-types')}function b(){return a}a.isRequired=a;var c={array:a,bool:a,func:a,number:a,object:a,string:a,symbol:a,any:a,arrayOf:b,element:a,instanceOf:b,node:a,objectOf:b,oneOf:b,oneOfType:b,shape:b};return c.checkPropTypes=d,c.PropTypes=c,c}},19:function(a){'use strict';a.exports='SECRET_DO_NOT_PASS_THIS_OR_YOU_WILL_BE_FIRED'},5:function(a){'use strict';function b(a){return function(){return a}}var c=function(){};c.thatReturns=b,c.thatReturnsFalse=b(!1),c.thatReturnsTrue=b(!0),c.thatReturnsNull=b(null),c.thatReturnsThis=function(){return this},c.thatReturnsArgument=function(a){return a},a.exports=c}});this.EXPORTED_SYMBOLS = ["PropTypes"];
\ No newline at end of file

Maybe don't change these minified shield-recipe-client files? It looks like these files are fixed on Github and then imported. At least ask the person who imported them last what to do.

Comment 6

[Mark Banner (:standard8)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Florian explained over IRC to me that the eslint changes are just making us
> use the default global behavior now, which is better, so that's fine.

Florian, can we leave the ESlint changes out, or are they required to get this patch to pass? The reason is by reverting to the global behaviour it actually makes things less strict.

In bug 1440379 I'm making it much clearer as to what the differences from the defaults actually are.

In some ways I'd like to head to vars="all" for everywhere (though its going to take a bit of work), so having some directories stay on "all" would be useful for now.

Comment 7

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #6)
> (In reply to Andrew McCreight [:mccr8] from comment #4)
> > Florian explained over IRC to me that the eslint changes are just making us
> > use the default global behavior now, which is better, so that's fine.
> 
> Florian, can we leave the ESlint changes out, or are they required to get
> this patch to pass?

Without the eslint changes, it fails by reporting 'EXPORTED_SYMBOLS' as an unused variable in these folders.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b599a95ce057
change this.EXPORTED_SYMBOLS back to var EXPORTED_SYMBOLS in JS modules, r=mccr8.

Comment 9

[Ed Lee :Mardak]

florian, Is this change going to be required? Activity Stream build, tests, and linting are quite broken now without the `this.* =`

k88hudson, I'm guessing the jsm-to-commonjs/esmodules need to be updated? Is that okay with all the `var`s?

Separately, we have a bunch of eslint errors for no-var, vars-on-top, no-unused-vars, no-multiple-empty-lines; but I suppose some can just be ignored/disabled.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b599a95ce057

Comment 11

[Florian Quèze [:florian]]

(In reply to Ed Lee :Mardak from comment #9)
> florian, Is this change going to be required?

Not really required. The goal here was just to revert to the style that was preferred before we had to introduce a B2G hack a couple years ago. I have no plan to add an eslint rule to enforce the 'var EXPORTED_SYMBOLS' style. What prompted me to do a mass change is that I saw review comments a couple times requesting that patch author do the this. -> var change while they were touching the line, and I figured doing it across the whole tree at once was more efficient.

> Activity Stream build, tests,
> and linting are quite broken now without the `this.* =`

This seems like a problem that should be fixed. While working on this bug I did my best to ensure I wouldn't break anything. I pushed to try and ran *all* tests. If your eslint rules are stricter than those we have by default on mozilla-central, you should consider making the linting in the mozilla-central directory match what you do elsewhere.

Comment 12

[Ed Lee :Mardak]

(In reply to Florian Quèze [:florian] from comment #11)
> > Activity Stream build, tests,
> > and linting are quite broken now without the `this.* =`
> This seems like a problem that should be fixed. While working on this bug I
> did my best to ensure I wouldn't break anything. I pushed to try and ran
> *all* tests.
Nod. Not your fault that Activity Stream has an odd build structure as we land built files that require npm. We've been in larger github/npm discussions that sound like we're heading towards npm more generally available in m-c, so I wonder what would have happened here assuming we were already building in-tree?

In particular, there's external devDependencies to get `this. -> var` working, so our build/automation would have failed in m-c but the fix would require getting the dependency updated. I suppose one simple way to unblock this bug would have been to ignore activity stream directory?

Comment 13

[Florian Quèze [:florian]]

(In reply to Ed Lee :Mardak from comment #12)
> I suppose one simple way to unblock
> this bug would have been to ignore activity stream directory?

Well, if tests had uncovered a problem I would have needinfo'ed you and we would then have discussed how to move forward.

Comment 14

[Kate Hudson :k88hudson]

Yes, this will require changes to the jsm-to-esmodules plugin. I'll file a bug to update it

Comment 15

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4002

Comment 16

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/defc6cfecb0b5d7090a08e466c224335a50cf38c
chore(mc): Port Bug 1440284 - change this.EXPORTED_SYMBOLS back to var EXPORTED_SYMBOLS in JS modules, r=mccr8. (#4002)