fix(platform): change isBrowser check to use Angular PLATFORM_ID #10659

CaerusKaru · 2018-04-02T07:01:41Z

Use the Angular PLATFORM_ID token as a more reliable check for
whether the current platform is browser. This is because the
previous check for a global document could be a false positive
if another application pollutes the global namespace

BREAKING CHANGE:

Platform now takes the PLATFORM_ID as an optional parameter in
its constructor. This will become a required parameter in v7

crisbeto · 2018-04-02T07:06:41Z

src/cdk/platform/platform.ts

@@ -59,4 +60,8 @@ export class Platform {
  // this and just place the Safari keyword in the userAgent. To be more safe about Safari every
  // Safari browser should also use Webkit as its layout engine.
  SAFARI: boolean = this.isBrowser && /safari/i.test(navigator.userAgent) && this.WEBKIT;
+
+  constructor(@Inject(PLATFORM_ID) private _platformId: Object) {


This is a breaking change, because it changes the constructor signature.

Totally agree, but I still think it's a change that eventually needs to go in. I'm going through the rest of the tests to update the signature

This still isn't the proper way to handle it though. Instead it should be marked as an optional parameter with a deletion target for 7.0.0 and it should fall back to the old way of checking.

Agreed, will change

crisbeto · 2018-04-02T07:45:19Z

src/cdk/platform/platform.ts

+  /**
+   * @deletion-target v7.0.0 remove optional decorator
+   */
+  constructor(@Optional() @Inject(PLATFORM_ID) private _platformId: Object) {


This still doesn't make it a non-breaking change. You also have to mark the _platformId as an optional param by adding a question mark.

Thought I did, my mistake. Will add, thanks for the catch

CaerusKaru · 2018-04-02T08:09:20Z

Note: this is failing on one test case for the InteractivityChecker. Unless there's a simple way to refactor the test to avoid the undefined error, I'll leave it as a TODO for when the Platform constructor parameter is no longer optional.

jelbourn · 2018-04-02T16:24:50Z

src/cdk/a11y/interactivity-checker/interactivity-checker.spec.ts

@@ -5,6 +5,7 @@ import {InteractivityChecker} from './interactivity-checker';
 describe('InteractivityChecker', () => {
  let testContainerElement: HTMLElement;
  let checker: InteractivityChecker;
+  // TODO: refactor this to be injected


This comment needs to explain why

https://github.com/angular/material2/blob/master/CODING_STANDARDS.md#write-useful-comments

Good catch, will add

jelbourn · 2018-04-02T16:26:27Z

src/cdk/platform/platform.ts

@@ -20,7 +21,8 @@ const hasV8BreakIterator = (typeof Intl !== 'undefined' && (Intl as any).v8Break
 @Injectable({providedIn: 'root'})
 export class Platform {
  /** Whether the Angular application is being rendered in the browser. */
-  isBrowser: boolean = typeof document === 'object' && !!document;
+  isBrowser: boolean = this._platformId ?
+    isPlatformBrowser(this._platformId) : typeof document === 'object' && !!document;


I'm not sure I see the benefit of making this change. If someone wants to shim the document to pretend this is the browser, why not respect that?

Because if they shim the document but not the navigator, like Domino does, all of the other checks result in errors

Not to mention that it might not be an intentional shim

You have to go through a bit more trouble to declare a global variable in Node though (e.g. global.document = document).

Yeah, but it's happening to me. This PR didn't just come out of nowhere. The Angular testing utilities are polluting the global namespace for Node tests (see here)

Can you add some commenting here that explains that? Especially the part about what Domino does

Sure, will add. Alternatively I can add a check for the navigator to each of the below checks. Let me know which you prefer

Using the platform id works, I just want people who look at this in the future to be able to take advantage of all of the information you picked up while figuring this out

jelbourn · 2018-04-02T16:28:19Z

src/cdk/platform/platform.ts

@@ -37,11 +39,11 @@ export class Platform {
  // Webkit is part of the userAgent in EdgeHTML, Blink and Trident. Therefore we need to
  // ensure that Webkit runs standalone and is not used as another engine's base.
  WEBKIT: boolean = this.isBrowser &&
-      /AppleWebKit/i.test(navigator.userAgent) && !this.BLINK && !this.EDGE && !this.TRIDENT;
+    /AppleWebKit/i.test(navigator.userAgent) && !this.BLINK && !this.EDGE && !this.TRIDENT;


The indent was right before (+4 for overflow)

Will fix here and other places

jelbourn · 2018-04-02T16:29:37Z

src/lib/tabs/tab-group.spec.ts

@@ -279,7 +279,7 @@ describe('MatTabGroup', () => {
        tick();

        tabs = component._tabs.toArray();
-        expect(tabs[3].origin).toBeGreaterThanOrEqual(0);
+        expect(tabs[3].origin! >= 0).toBe(true);


Can you explain what's going on here?

This was left over from when I was reviewing #10642, will remove

* Use the Angular PLATFORM_ID token as a more reliable check for whether the current platform is browser. This is because the previous check for a global document could be a false positive if another application pollutes the global namespace BREAKING CHANGE: * Platform now takes the PLATFORM_ID as an optional parameter in its constructor. This will become a required parameter in v7

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-08T17:03:17Z

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.}

CaerusKaru requested review from devversion and jelbourn as code owners April 2, 2018 07:01

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

CaerusKaru force-pushed the platform branch 2 times, most recently from 10835f6 to ae598a8 Compare April 2, 2018 07:03

crisbeto reviewed Apr 2, 2018

View reviewed changes

CaerusKaru force-pushed the platform branch from ae598a8 to 26ada51 Compare April 2, 2018 07:39

CaerusKaru requested review from andrewseguin and mmalerba as code owners April 2, 2018 07:39

crisbeto reviewed Apr 2, 2018

View reviewed changes

CaerusKaru force-pushed the platform branch 3 times, most recently from cc13cef to 6cdfdca Compare April 2, 2018 07:54

CaerusKaru force-pushed the platform branch from 6cdfdca to d492fd4 Compare April 2, 2018 08:10

jelbourn reviewed Apr 2, 2018

View reviewed changes

CaerusKaru force-pushed the platform branch from d492fd4 to 7c123c5 Compare April 2, 2018 17:00

CaerusKaru force-pushed the platform branch from 7c123c5 to 62b50a5 Compare April 2, 2018 17:01

jelbourn approved these changes Apr 9, 2018

View reviewed changes

jelbourn added P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Apr 9, 2018

tinayuangao merged commit f023579 into angular:master Apr 11, 2018

CaerusKaru deleted the platform branch April 11, 2018 17:50

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

fix(platform): change isBrowser check to use Angular PLATFORM_ID #10659

fix(platform): change isBrowser check to use Angular PLATFORM_ID #10659

Conversation

CaerusKaru commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

CaerusKaru commented Apr 2, 2018

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!

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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

CaerusKaru commented Apr 2, 2018 •

edited

Loading