fix(all): remove all direct use of browser globals #10826

graingert · 2017-08-01T16:11:27Z

use injectable $window and $document instead.

This removes the private provider $$mdMetaProvider because it used to access document before it was available. Meta headers can only be changed via the $$mdMeta service.
This updates and deprecates $mdThemingProvider.enableBrowserColor() to queue up color changes and dispatch them in the actual service when $$mdMeta is available.
This adds support for defining panel presets with an injectable function for the case where you want to
define a preset that sets attachTo: angular.element($document[0].body) inside a config function.
This replaces all alert() calls with $mdDialog.show($mdDialog.alert()...); calls

This depends on eslint to find the misuse of global variables: #10824

What are $window and $document?

According to the AngularJS documentation: https://docs.angularjs.org/api/ng/service/$window

A reference to the browser's window object. While window is globally available in JavaScript, it causes testability problems, because it is a global variable. In AngularJS we always refer to it through the $window service, so it may be overridden, removed or mocked for testing.

$document is similar

Why is this needed?

Before $window, window and document, $document were being used interchangeably, this PR ensures consistency.
using an injectable $window and $document allows ...
- ... multiple angular apps to co-exist without destructively altering each other.
- ... users of angular-material to patch out those values in tests.
- ... angular-material to work in non-browser environments without those globals: extensions, webworkers and node.
- ... material-tools to work without virtual_context chore: remove virtual_context and override the $window and $document providers instead material-tools#31

src/components/swipe/demoBasicUsage/index.html

graingert · 2017-08-04T12:47:10Z

@jelbourn @tinayuangao I rebased this on master just now.

graingert · 2017-08-16T13:29:08Z

@mgol in reply to your comment #10824 (comment) this builds on #10824 if you merge that, those unrelated commits will disappear

graingert · 2017-08-16T13:36:58Z

There should be one PR for ESLint setup, then another for any $window-related changes etc.

That is indeed how I've split the PRs.

graingert · 2017-08-16T16:52:37Z

@ThomasBurleson @EladBezalel any chance of a review here?

Frank3K · 2017-08-23T17:56:06Z

@graingert: it looks like there are lots of other commits in this PR. Can you fix this?

graingert · 2017-08-23T18:03:26Z

@Frank3K those are just the extra commits from #10824 when that's merged the commits will disappear from here

Frank3K · 2017-08-24T17:25:09Z

@graingert: I know, but I think the PR's will not get pulled before they are cleaned up...

graingert · 2017-08-24T17:26:50Z

@Frank3K these PRs are clean. They just depend on each other.

ThomasBurleson · 2017-09-05T02:51:42Z

@graingert - This PR will not be accepted because the scope of the changes are too broad.
Please resubmit with small, granular PRs and we will gladly review those for inclusion in 1.1.6.

graingert · 2017-09-05T07:07:45Z

@ThomasBurleson can you have a look at #10824 when that's merged it will automatically unbroarden this pr

relay on per application state.

Angular code should not use global state, it should be managed by providers. expose isDisabled() etc on $mdTheming remove theme disabling logic from md-themes-disabled directive (it has no effect after generateAllThemes is run, because that's the only location that reads themeConfig.disableTheming) check $document for md-themes-disabled in generateAllThemes.

58 failing tests

graingert · 2018-08-20T08:26:53Z

@Splaktar well I've rebased it

graingert · 2018-08-20T08:39:13Z

@Splaktar ok so all the tests pass

Splaktar

I made it about halfway through reviewing this. Many of the changes are harmless, but there are a number of cases where issues pop up.

In those cases, I observed exceptions in the demos that need to be resolved.

Please address the issues that I called out, manually re-test all demos that had either a demo change or component change, and then let me know and I'll review again.

Splaktar · 2018-09-04T16:35:46Z

src/components/autocomplete/demoBasicUsage/index.html

@@ -18,7 +18,7 @@
        </md-item-template>
        <md-not-found>
          No states matching "{{ctrl.searchText}}" were found.
-          <a ng-click="ctrl.newState(ctrl.searchText)">Create a new one!</a>
+          <md-button ng-click="ctrl.newState(ctrl.searchText, $event)">Create a new one!</md-button>


The new behavior here is quite buggy.

Let's completely remove the concept of a link or button in the md-not-found for this demo.

Splaktar · 2018-09-04T16:36:38Z

src/components/autocomplete/demoBasicUsage/script.js

+      $mdDialog.show(
+        $mdDialog
+          .alert()
+          .title('state creation failed.')


Let's completely remove the concept of a link or button in the md-not-found, and thus this newState() function for this demo.

Splaktar · 2018-09-04T16:46:07Z

src/components/chips/demoContactChips/script.js

+    }
+
+    return function($document, $q, $window) {
+      if (prom) {


What does prom stand for? Perhaps this can be renamed to be more clear.

Splaktar · 2018-09-04T16:48:42Z

src/components/chips/demoContactChips/script.js

    var self = this;
    var pendingSearch, cancelSearch = angular.noop;
    var lastSearch;

-    self.allContacts = loadContacts();
+    loader($document, $q, $window).then(function (CryptoJS) {


This throws:

TypeError: Cannot read property 'then' of undefined at DemoCtrl (docs-demo-scripts.js:589)

Urgh, probably a missing return

Splaktar · 2018-09-04T16:59:07Z

src/components/icon/js/iconService.js

@@ -400,7 +403,7 @@ function ConfigurationItem(url, viewBoxSize) {
 */

 /* @ngInject */
-function MdIconService(config, $templateRequest, $q, $log, $mdUtil, $sce) {
+function mdIconService(config, $templateRequest, $q, $log, $mdUtil, $sce, $window) {


Why did the casing of the name need to change here?

Probably a rebase error

Splaktar · 2018-09-04T17:20:11Z

src/components/panel/demoBasicUsage/script.js

  this.selected.favoriteDessert = dessert;
  this._mdPanelRef && this._mdPanelRef.close().then(function() {
-    angular.element(document.querySelector('.demo-menu-open-button')).focus();
+    angular.element($document[0].querySelector('.demo-menu-open-button')).focus();


This throws

TypeError: Cannot read property '0' of undefined at docs-demo-scripts.js:2220

Splaktar · 2018-09-04T17:23:19Z

src/components/panel/demoPanelAnimations/script.js

@@ -6,7 +6,7 @@ angular.module('panelAnimationsDemo', ['ngMaterial'])
    .controller('DialogCtrl', DialogCtrl);


-function AnimationCtrl($mdPanel) {
+function AnimationCtrl($mdPanel, $document) {
  this._mdPanel = $mdPanel;
  this.openFrom = 'button';


Opening the dialog throws

TypeError: Cannot read property '0' of undefined at AnimationCtrl.showDialog (docs-demo-scripts.js:2440)

Splaktar · 2018-09-04T17:25:57Z

src/components/panel/panel.js

@@ -1204,12 +1204,19 @@ MdPanelService.prototype.open = function(preset, config) {
 * @returns {!Object} The preset configuration object.
 */
 MdPanelService.prototype._getPresetByName = function(preset) {
-  if (!this._presets[preset]) {
+  var $injector = this._$injector;
+  var v = this._presets[preset];


What does v stand for? Can you please give this a more meaningful name?

It's an arbitrary value because I didn't know what it was for.

Maybe the preset -> presetName and v -> preset?

That sounds good. It is much more clear.

Splaktar · 2018-09-04T17:28:36Z

src/components/radioButton/demoBasicUsage/script.js

+          .alert()
+          .title('Action')
+          .textContent('submit')
+          .event($event)


This throws

TypeError: $mdDialog.alert(...).title(...).textContent(...).event is not a function at m.$scope.submit (docs-demo-scripts.js:2746)

I'm probably not understanding how $mdDialog works here, would be good for an angular material dev to take a look, please?

It's .targetEvent($event)

Splaktar · 2018-09-04T17:29:32Z

src/components/datepicker/js/calendarMonth.js

+   * Change the selected date in the calendar (ngModel value has already been changed).
+   * @param {Date} date
+   */
+  CalendarMonthCtrl.prototype.changeSelectedDate = function(date) {


Where did this come from?

I probably resurrected this by accident in a rebase

graingert · 2018-09-04T17:44:15Z

The $document[0] not working is really worrying. This should be injected by angularjs as a jqlite element of document

graingert · 2018-09-04T17:44:51Z

@Splaktar can you take over this PR and make those changes? I'm not using AngularJS anymore so don't have much time to work on this

Splaktar · 2018-09-04T18:03:48Z

OK, I'll look into creating a new PR, which I'll need you to post in to grant approval to use your commits. I probably won't have time to do this real soon.

Thank you for getting a start here though.

graingert · 2018-09-04T18:41:50Z

@Splaktar I've enabled edits to this PR from maintainers, so you should be able to push to this branch

Splaktar · 2020-12-06T02:28:49Z

Thank you for all of the work on this effort. Unfortunately, this has been determined to be too risky to try to get merged at this point.

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Aug 1, 2017

graingert force-pushed the no-browser-globals branch 4 times, most recently from da46b1f to 0857fdb Compare August 2, 2017 00:47

graingert changed the title ~~WIP: remove all direct use of browser globals~~ remove all direct use of browser globals Aug 2, 2017

graingert changed the title ~~remove all direct use of browser globals~~ chore(all): remove all direct use of browser globals Aug 2, 2017

graingert changed the title ~~chore(all): remove all direct use of browser globals~~ fix(all): remove all direct use of browser globals Aug 2, 2017

graingert force-pushed the no-browser-globals branch 3 times, most recently from c1939ec to 2d0bfea Compare August 2, 2017 01:23

nobitagit reviewed Aug 2, 2017

View reviewed changes

src/components/swipe/demoBasicUsage/index.html Outdated Show resolved Hide resolved

graingert force-pushed the no-browser-globals branch 3 times, most recently from 1f158f9 to dbe7650 Compare August 4, 2017 12:45

graingert mentioned this pull request Aug 4, 2017

chore: remove virtual_context and override the $window and $document providers instead angular/material-tools#31

Open

graingert mentioned this pull request Aug 16, 2017

chore(eslint): Configure ESLint Fixes #9261 #10824

Merged

graingert force-pushed the no-browser-globals branch 2 times, most recently from db24910 to fe7f6e4 Compare August 16, 2017 13:03

ThomasBurleson closed this Sep 5, 2017

Thomas Grainger and others added 7 commits August 20, 2018 09:15

no global state in $$mdLayout

3335096

relay on per application state.

support defining panel presets with a function

59e9f5f

fix(all) use $window and $document over their respective globals.

8a27c80

fix $mdThemingProvider.enableBrowserColor queue

608d4f0

58 failing tests

ensure all tests pass

575f995

fix newly introduced window.document calls

d88aab9

graingert force-pushed the no-browser-globals branch from 4e4befd to d88aab9 Compare August 20, 2018 08:26

graingert added 2 commits August 20, 2018 09:29

fix undefined MutationObserver

5cc67ed

fix theming service case

c1da2fb

Splaktar added needs: squash commits and removed needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved labels Sep 4, 2018

Splaktar suggested changes Sep 4, 2018

View reviewed changes

Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 4, 2018

Splaktar modified the milestones: 1.1.11, - Backlog Sep 4, 2018

Splaktar modified the milestones: - Backlog, 1.2.0 Jul 6, 2020

Splaktar modified the milestones: 1.2.0, - Backlog Jul 28, 2020

Splaktar added the resolution: too risky There is too much risk or disruption with this proposal for our limited resources. label Dec 6, 2020

Splaktar removed this from the - Backlog milestone Dec 6, 2020

Splaktar closed this Dec 6, 2020

fix(all): remove all direct use of browser globals #10826

fix(all): remove all direct use of browser globals #10826

Conversation

graingert commented Aug 1, 2017 • edited Loading

What are $window and $document?

Why is this needed?

graingert commented Aug 4, 2017

graingert commented Aug 16, 2017

graingert commented Aug 16, 2017

graingert commented Aug 16, 2017

Frank3K commented Aug 23, 2017

graingert commented Aug 23, 2017

Frank3K commented Aug 24, 2017

graingert commented Aug 24, 2017

ThomasBurleson commented Sep 5, 2017

graingert commented Sep 5, 2017

graingert commented Aug 20, 2018

graingert commented Aug 20, 2018

Splaktar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Splaktar Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Sep 4, 2018

graingert commented Sep 4, 2018

Splaktar commented Sep 4, 2018

graingert commented Sep 4, 2018

Splaktar commented Dec 6, 2020

graingert commented Aug 1, 2017 •

edited

Loading

Splaktar Sep 4, 2018 •

edited

Loading