fix(universal): gate several browser-specific bits on being on the browser #4251

jelbourn · 2017-04-24T22:43:16Z

Related to #308

This is really a temporary fix until we have a secondary @angular/material/server entry-point which can contain things like NoopScrollDispatcher, NoopRippleRenderer, etc. I don't want to add these classes to the main @angular/material package both because they would eventually move and they would add bloat for client payload.

Also removed a few unnecessary [class] bindings while I was around

mmalerba

1 nit

mmalerba · 2017-04-24T23:11:58Z

src/lib/core/style/focus-origin-monitor.ts

@@ -126,6 +132,11 @@ export class FocusOriginMonitor {

  /** Register necessary event listeners on the document and window. */
  private _registerDocumentEvents() {
+    // Do nothing if we're not on the browser platform.
+    if (!isBrowser()) {
+      return Observable.of();


return; (method isn't expected to return anything)

crisbeto · 2017-04-25T06:38:52Z

src/lib/button/button.ts

+    // If not on the browser, say that there are none of the attributes present.
+    // Since these only affect how the ripple displays (and ripples only happen on the client),
+    // detecting these attributes isn't necessary when not on the browser.
+    if (!isBrowser()) {


Might be nicer if the return below was return isBrowser() && unprefixedAttributeNames.some....

crisbeto · 2017-04-25T06:45:55Z

src/lib/core/overlay/position/viewport-ruler.ts

@@ -51,6 +54,12 @@ export class ViewportRuler {
   * @param documentRect
   */
  getViewportScrollPosition(documentRect = this._documentRect) {
+    // Cache the document bounding rect so that we don't recompute it for multiple calls.
+    if (!documentRect) {
+      this._cacheViewportGeometry();


This could be a little shorter if we made _cacheViewportGeometry return the documentRect and then changed the method definition to:

getViewportScrollPosition(documentRect = this._cacheViewportGeometry()) { // continue as usual }

That would then no longer be using the cached version if there is one.

crisbeto · 2017-04-25T06:48:12Z

src/lib/core/platform/browser.ts

@@ -0,0 +1,4 @@
+/** Gets whether the current platform is a web browser (versus node). */
+export function isBrowser() {
+  return typeof document === 'object';


A couple of things:

Technically shouldn't be an issue, but typeof document === 'object' will be true if document is null.

Perhaps this should be a const, instead of a function?

Also I was doing these checks inline in a few places, I don't know whether we shouldn't switch them to use isBrowser with this PR or in a follow-up.

Changed check to typeof document === 'object' && document;

We can get other places in a follow-up.

jelbourn · 2017-04-25T18:34:16Z

Replaced implementation from yesterday with a new approach that adds isBrowser to the existing Platform injectable. This will make it easier to test/fake in places where that would be necessary.

This led to a tangent of having to fix some modules and tests that were adding providers directly instead of importing the module where the provider lives.

crisbeto

LGTM

mmalerba · 2017-04-26T16:11:39Z

LGTM, needs rebase though

jelbourn · 2017-04-26T16:51:29Z

Rebased

jelbourn · 2017-04-26T18:38:18Z

@mmalerba fixed circular dep

mmalerba · 2017-04-28T17:29:39Z

@jelbourn needs rebase and investigation into failing test

jelbourn · 2017-04-28T20:34:51Z

@mmalerba after I rebased the error went away

mmalerba · 2017-04-28T21:22:18Z

@jelbourn sorry, needs more rebasing

jelbourn · 2017-04-29T00:11:44Z

Rebased again- weirdly, git couldn't auto-resolve the conflicts, but WebStorm could

jelbourn · 2017-05-03T22:10:03Z

@andrewseguin rebased again

aitboudad · 2017-05-07T15:07:50Z

tried on my side and I got some little issues:

missing rxjs of import
navigator is not defined in Platform

mmalerba · 2017-05-08T16:42:25Z

still has some lint issues and failing tests too

browser Related to angular#308

jelbourn · 2017-05-10T21:53:18Z

@kara rebased again and green again

With angular#4251 the `ScrollDispatchModule` has been created and it is used by a lot of different components. The class is already exported inside of the file but the core package doesn't export it to the public.

With #4251 the `ScrollDispatchModule` has been created and it is used by a lot of different components. The class is already exported inside of the file but the core package doesn't export it to the public.

angular-automatic-lock-bot · 2019-09-07T04:01:28Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

jelbourn added the pr: needs review label Apr 24, 2017

jelbourn assigned crisbeto and mmalerba Apr 24, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 24, 2017

mmalerba approved these changes Apr 24, 2017

View reviewed changes

mmalerba added pr: lgtm and removed pr: needs review labels Apr 24, 2017

crisbeto reviewed Apr 25, 2017

View reviewed changes

jelbourn force-pushed the universal branch from 5614886 to 22a9862 Compare April 25, 2017 18:29

jelbourn added pr: needs review and removed pr: lgtm labels Apr 25, 2017

crisbeto approved these changes Apr 25, 2017

View reviewed changes

mmalerba added pr: lgtm and removed pr: needs review labels Apr 26, 2017

jelbourn force-pushed the universal branch from 22a9862 to a4e6313 Compare April 26, 2017 16:50

jelbourn added the action: merge The PR is ready for merge by the caretaker label Apr 26, 2017

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Apr 28, 2017

jelbourn force-pushed the universal branch from ee886cc to fe3a57d Compare April 28, 2017 19:14

jelbourn added the action: merge The PR is ready for merge by the caretaker label Apr 28, 2017

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Apr 28, 2017

jelbourn force-pushed the universal branch from fe3a57d to b9b5887 Compare April 29, 2017 00:11

jelbourn added the action: merge The PR is ready for merge by the caretaker label Apr 29, 2017

jelbourn force-pushed the universal branch from b9b5887 to 93d411c Compare May 1, 2017 21:02

jelbourn mentioned this pull request May 3, 2017

Support Angular Universal (with CI job to test all components) #308

Closed

jelbourn force-pushed the universal branch from 93d411c to 6c17cca Compare May 3, 2017 22:10

mmalerba removed the action: merge The PR is ready for merge by the caretaker label May 8, 2017

jelbourn force-pushed the universal branch 4 times, most recently from 85d3ba4 to 27778ad Compare May 10, 2017 21:25

jelbourn added 3 commits May 10, 2017 14:52

fix(universal): gate several browser-specific bits on being on the

bea3885

browser Related to angular#308

refactor to use existing Platform class

89a4108

fix scroll dispatcher circular dep

359ce84

jelbourn force-pushed the universal branch from 27778ad to 359ce84 Compare May 10, 2017 21:52

jelbourn added the action: merge The PR is ready for merge by the caretaker label May 10, 2017

qdouble mentioned this pull request May 11, 2017

Angular material is not working on master branch (universal + material) qdouble/angular-webpack-starter#278

Closed

kara merged commit f27df86 into angular:master May 11, 2017

devversion mentioned this pull request May 12, 2017

feat(core): expose ScrollDispatchModule #4501

Merged

ThomasBurleson mentioned this pull request Jun 23, 2017

Using flex-layout with Angular Universal angular/flex-layout#187

Closed

jelbourn deleted the universal branch September 13, 2017 04:38

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(universal): gate several browser-specific bits on being on the browser #4251

fix(universal): gate several browser-specific bits on being on the browser #4251

jelbourn commented Apr 24, 2017

mmalerba left a comment

mmalerba Apr 24, 2017

crisbeto Apr 25, 2017

crisbeto Apr 25, 2017 •

edited

Loading

jelbourn Apr 25, 2017

crisbeto Apr 25, 2017

jelbourn Apr 25, 2017

jelbourn commented Apr 25, 2017 •

edited

Loading

crisbeto left a comment

mmalerba commented Apr 26, 2017

jelbourn commented Apr 26, 2017

jelbourn commented Apr 26, 2017

mmalerba commented Apr 28, 2017

jelbourn commented Apr 28, 2017

mmalerba commented Apr 28, 2017

jelbourn commented Apr 29, 2017

jelbourn commented May 3, 2017

aitboudad commented May 7, 2017

mmalerba commented May 8, 2017

jelbourn commented May 10, 2017

angular-automatic-lock-bot bot commented Sep 7, 2019

fix(universal): gate several browser-specific bits on being on the browser #4251

fix(universal): gate several browser-specific bits on being on the browser #4251

Conversation

jelbourn commented Apr 24, 2017

mmalerba left a comment

Choose a reason for hiding this comment

mmalerba Apr 24, 2017

Choose a reason for hiding this comment

crisbeto Apr 25, 2017

Choose a reason for hiding this comment

crisbeto Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

jelbourn Apr 25, 2017

Choose a reason for hiding this comment

crisbeto Apr 25, 2017

Choose a reason for hiding this comment

jelbourn Apr 25, 2017

Choose a reason for hiding this comment

jelbourn commented Apr 25, 2017 • edited Loading

crisbeto left a comment

Choose a reason for hiding this comment

mmalerba commented Apr 26, 2017

jelbourn commented Apr 26, 2017

jelbourn commented Apr 26, 2017

mmalerba commented Apr 28, 2017

jelbourn commented Apr 28, 2017

mmalerba commented Apr 28, 2017

jelbourn commented Apr 29, 2017

jelbourn commented May 3, 2017

aitboudad commented May 7, 2017

mmalerba commented May 8, 2017

jelbourn commented May 10, 2017

angular-automatic-lock-bot bot commented Sep 7, 2019

crisbeto Apr 25, 2017 •

edited

Loading

jelbourn commented Apr 25, 2017 •

edited

Loading