refactor(jshint): don't assume browser-only globals by mgol · Pull Request #14345 · angular/angular.js

mgol · 2016-03-30T12:34:53Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature: We're no longer assuming browser globals in src/**/*.js which will prevent issues like #13442 from happening in the future.

What is the current behavior? (You can also link to an open issue here)

Currently browser globals are assumed. This sometimes breaks tools like jsdom that make them available under window but not as globals; see e.g. #13442.

What is the new behavior (if this is a feature change)?

For the browser these changes shouldn't matter. In other environments browser globals are now taken from the window variable instead of the global.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
~~Tests for the changes have been added (for bug fixes / features)~~
~~Docs have been added / updated (for bug fixes / features)~~

Other information:

Fixes #13442

mgol · 2016-03-30T12:35:05Z

I also updated JSHint.

mgol · 2016-03-30T13:03:39Z

@petebacondarwin I see JSHint failures in the docs gulp task. Which JSHint config is used to lint build/docs/**/*.js?

petebacondarwin · 2016-03-30T13:21:46Z

@mgol I guess it is just the top level .jshintrc in the root of the project.

petebacondarwin · 2016-03-30T13:29:11Z

If you want we could copy a docs specific .jshintrc into the build/docs folder as part of the gulp assets task.

mgol · 2016-03-30T13:38:03Z

Where are globals like by taken from? It surprised me to see errors like:

'by' is not defined

gkalpak · 2016-03-31T06:37:00Z

by should be from Protractor

WRT .jshintrc, I believe it's better to take a more granular approach, specifying globals in the directories they are needed. E.g. don't have something in the top level if we only need it in src/ or test/ etc.

petebacondarwin · 2016-03-31T16:22:21Z

@gkalpak - in that case we need dgeni to create and provide such a file for every example that contains e2e tests.

gkalpak · 2016-04-01T06:10:23Z

Why is that ? Can't we just put the required globals in a parent directory's .jshintrc ?

petebacondarwin · 2016-04-01T15:53:07Z

OK, so I forgot that the e2e tests all go in their own folder.
The relevant folders we have are:

docs
 - js
 - examples
 - ptore2e

So we should copy a baseline .jshintrc in the docs folder, then perhaps a specialized one inside the ptoe2e.

mgol · 2016-04-01T16:28:49Z

So we should copy a baseline .jshintrc in the docs folder

We can extend a common one, no need to copy all the settings. :)

lgalfaso · 2016-04-01T16:37:57Z

Would it be possible to use $window in the cases that this is possible?

mgol · 2016-04-01T18:26:42Z

@lgalfaso It would but I'm pretty sure it'd break a lot of tests as people are mocking $window and not providing all necessary fields like setTimeout so I don't think that's something we should attempt in 1.5.x.

Also, we're currently providing globals window & document as parameters to the factory; we'd need to drop document from there and always use $window.document.

We might consider all that for 1.6 but I wanted to land something in 1.5.x and it seems too risky to me. WDYT?

lgalfaso · 2016-04-01T19:13:50Z

Fair point, let's keep window

On Friday, April 1, 2016, Michał Gołębiowski notifications@github.com
wrote:

@lgalfaso https://github.com/lgalfaso It would but I'm pretty sure it'd
break a lot of tests as people are mocking $window and not providing all
necessary fields like setTimeout so I don't think that's something we
should attempt in 1.5.x.

Also, we're currently providing globals window & document as parameters
to the factory

angular.js/src/angular.suffix

Line 5 in b54634d

})(window, document);

;
we'd need to drop document from there and always use $window.document.

We might consider all that for 1.6 but I wanted to land something in 1.5.x
and it seems too risky to me. WDYT?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14345 (comment)

gkalpak · 2016-04-01T20:33:13Z

@mgol, now that you mention it, couldn't we just change document to window.document in angular.suffix#L5 and leave it as just document in most other places ?

mgol · 2016-04-01T20:45:01Z

@gkalpak I've already talked with @petebacondarwin about that. :) I plan to apply this change.

mgol · 2016-04-04T21:47:59Z

So, I've been wondering about this change. Since I'm removing "browser": true I need to specify each browser global that should be used directly in .jshintrc. But that would mean I need to add document there as well. If we go this route, we might also want to add window.setTimeout & friends to the factory parameters in

angular.js/src/angular.suffix

Line 5 in b54634d

})(window, document);

but then they'd need to be added there as well... Perhaps we need one base .jshintrc for all source files that would declare these assumed browser globals? Or maybe we should just prefix all browser globals with window. and not do all that? Thoughts?

petebacondarwin · 2016-04-05T06:46:07Z

Don't we have that situation right now?
The .jshintrc in the src folder is responsible (really) for the ng folder source files and the top level source files such as Angular.js and jqLite.js. Then each of the modules has its own .jshintrc. Each of these extends the jshintrc-base file.

mgol · 2016-04-05T09:36:38Z

The .jshintrc-base file is a base for the whole project but I want the few browser globals to only be defined in code run in browsers, not e.g. Node. We could perhaps get a separate .jshintrc-base created under src/ that would extend the main base config & add those globals from the factory and then extend this file?

mgol · 2016-04-05T09:38:18Z

OTH, as @lgalfaso mentioned, ideally we'd use $window where appropriate but we can't do it that way before 1.6.0. But if we want to do it in the end, keeping anything except window in the factory seems backwards. So we should decide if we want to change it in 1.6.0 and if the answer's yes I'd remove document from the factory & just prefix every non-core JS browser global with window..

petebacondarwin · 2016-04-06T11:00:38Z

@mgol I think that your suggestion based on moving to using $window everywhere in 1.6 and prefixing all browser globals with window in 1.5 sounds reasonable. Do you want to have a go at that and see if it turns up any difficulties?

mgol · 2016-04-06T11:18:50Z

@petebacondarwin on it!

mgol · 2016-04-06T12:22:02Z

PR updated.

mgol · 2016-04-06T12:44:50Z

All tests have passed this time.

petebacondarwin · 2016-04-06T14:12:26Z

Sweet!

petebacondarwin · 2016-04-06T14:14:40Z

docs/gulpfile.js



-gulp.task('doc-gen', ['bower'], function() {
+gulp.task('doc-gen', ['bower'], function(cb) {


I don't think we need the callback here

Right; I forgot to remove it after backtracking from a previous idea. :)

petebacondarwin · 2016-04-06T14:18:02Z

src/angular.prefix

 * License: MIT
 */
-(function(window, document, undefined) {
+(function(window) {


I think that having undefined as a parameter, which is not assigned, is actually a clever way to ensure that we have the "real" undefined inside the closure. So I am for leaving this as a parameter.

What would it guard against, though? We've used to have the same parameter in jQuery but we removed it; if someone created a variable undefined that is not equal to undefined, basically everything would be broken.

IOW, shadowning undefined with sth else is more or less equivalent to shadowing any other builtin like Object, Array, Object.prototype methods etc. There are lots of ways in which someone can create a broken environment in which Angular breaks. I don't think it buys us much to defend just against shadowing undefined and it's impossible to guard against everything.

var old_undefined = undefined; undefined = {}; function moo(undefined) { console.log(undefined === old_undefined); } moo();

I know what it does. :) What I'm saying is that it doesn't really buy us much; there are thousands other APIs that, if messed with, will break Angular and we don't protect against such changes. Moreover, I think it's more likely that someone will break Object.prototype for us than that they'll shadow undefined. I feel this undefined parameter is only providing a false sense of security.

It saves us from using void 0 everywhere, no?
I agree it doesn't save us from the other stuff, but we do have various bits of defensive code in place in the case that people do override parts of the Object.prototype.
What is the benefit in removing it?

It saves us from using void 0 everywhere, no?

I'd just rely on the undefined from the outer scope. Note that it's not even possible in ES5 to overwrite undefined, it's immutable even in sloppy mode (the only difference is that in strict mode trying to overwrite undefined throws while in sloppy mode it's just a noop). The only way this could backfire is if Angular was loaded not via a regular script tag but wrapped in a separate closure which shadowed undefined. But that requires modifying the source and if someone does that we've already lost the battle.

I agree it doesn't save us from the other stuff, but we do have various bits of defensive code in place in the case that people do override parts of the Object.prototype.

Since the global undefined is immutable if Angular is loaded normally so that its undefined points to the global one no one is able to modify that; shadowing is also impossible after the fact.

What is the benefit in removing it?

There is no huge benefit, perhaps a tiny size decrease. If you feel strongly about it, I'll restore it. :) I just think it doesn't really solve any problem.

OK, you convinced me :-)

Fixes angular#13442

petebacondarwin · 2016-04-06T17:11:14Z

OK, LGTM - please merge

(cherry-picked from ddad264) Fixes #13442 Closes #14345

gkalpak · 2016-04-07T22:26:11Z

👍

Regarding the using $window in 1.6.x, won't that break lots of tests relying on a mocked $window ?

mgol · 2016-04-07T22:49:12Z

@gkalpak #14345 (comment) :P

That's why I didn't want to land it for 1.5. As for 1.6, we can still discuss.

googlebot added cla: yes labels Mar 30, 2016

mgol mentioned this pull request Mar 30, 2016

Uncaught ReferenceError: Node is not defined #13442

Closed

mgol force-pushed the non-browser branch from d780cb6 to 5cd6ac4 Compare March 30, 2016 12:53

mgol force-pushed the non-browser branch from 5cd6ac4 to 29b65d6 Compare April 6, 2016 12:20

petebacondarwin reviewed Apr 6, 2016
View reviewed changes

refactor(jshint): don't assume browser-only globals

a5a8dae

Fixes angular#13442

mgol force-pushed the non-browser branch from 29b65d6 to a5a8dae Compare April 6, 2016 16:13

mgol closed this in ddad264 Apr 6, 2016

mgol deleted the non-browser branch April 6, 2016 18:36

mgol added a commit that referenced this pull request Apr 6, 2016

refactor(jshint): don't assume browser-only globals

f1bb836

(cherry-picked from ddad264) Fixes #13442 Closes #14345

gkalpak mentioned this pull request Oct 29, 2016

Uncaught ReferenceError: Node is not defined #15329

Closed

gkalpak mentioned this pull request Jan 17, 2017

Remove unneeded window reference #15610

Closed

3 tasks



		gulp.task('doc-gen', ['bower'], function() {
		gulp.task('doc-gen', ['bower'], function(cb) {

Conversation

mgol commented Mar 30, 2016

Uh oh!

mgol commented Mar 30, 2016

Uh oh!

mgol commented Mar 30, 2016

Uh oh!

petebacondarwin commented Mar 30, 2016

Uh oh!

petebacondarwin commented Mar 30, 2016

Uh oh!

mgol commented Mar 30, 2016

Uh oh!

gkalpak commented Mar 31, 2016

Uh oh!

petebacondarwin commented Mar 31, 2016

Uh oh!

gkalpak commented Apr 1, 2016

Uh oh!

petebacondarwin commented Apr 1, 2016

Uh oh!

mgol commented Apr 1, 2016

Uh oh!

lgalfaso commented Apr 1, 2016

Uh oh!

mgol commented Apr 1, 2016

Uh oh!

lgalfaso commented Apr 1, 2016

Uh oh!

gkalpak commented Apr 1, 2016

Uh oh!

mgol commented Apr 1, 2016

Uh oh!

mgol commented Apr 4, 2016

Uh oh!

petebacondarwin commented Apr 5, 2016

Uh oh!

mgol commented Apr 5, 2016

Uh oh!

mgol commented Apr 5, 2016

Uh oh!

petebacondarwin commented Apr 6, 2016

Uh oh!

mgol commented Apr 6, 2016

Uh oh!

mgol commented Apr 6, 2016

Uh oh!

mgol commented Apr 6, 2016

Uh oh!

petebacondarwin commented Apr 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Apr 6, 2016

Uh oh!

gkalpak commented Apr 7, 2016

Uh oh!

mgol commented Apr 7, 2016

Uh oh!

Reviewers

Assignees

Labels