Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(animations): false positive when detecting Node in Webpack builds #35134

Closed
wants to merge 1 commit into from
Closed

fix(animations): false positive when detecting Node in Webpack builds #35134

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/animations/browser/src/render/shared.ts
View file
Open in desktop
Expand Up @@ -15,12 +15,17 @@ import {AnimationDriver} from '../../src/render/animation_driver';
// types. `process` is just declared locally here as a result.
declare const process: any;

export function isBrowser() {
export function isBrowser(): boolean {
return (typeof window !== 'undefined' && typeof window.document !== 'undefined');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the ( and ) parenthesis needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they are. I didn't put it in, I just added a return type to the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (typeof ... ) are optional and are somewhat useful in increasing the readability of the code because they mean, what follows is a bit complex so we put it into () to warn you.

}

export function isNode() {
return (typeof process !== 'undefined');
export function isNode(): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking out loud: shouldn't somehow isNode() === !isBrowser()? I wonder if we can get away with one function only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the only place isBrowser is being used, we can't rely on !isNode, because it won't cover workers.

// Checking only for `process` isn't enough to identify whether or not we're in a Node
// environment, because Webpack by default will polyfill the `process`. While we can discern
// that Webpack polyfilled it by looking at `process.browser`, it's very Webpack-specific and
// might not be future-proof. Instead we look at the stringified version of `process` which
// is `[object process]` in Node and `[object Object]` when polyfilled.
return typeof process !== 'undefined' && {}.toString.call(process) === '[object process]';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this JavaScript code is valid? When I replace process with window I get the following error:
Screen Shot 2020-02-03 at 14 00 16

Copy link
Member Author

@crisbeto crisbeto Feb 5, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for me. It passed all the CI checks as well.
Cmder_2020-02-05_19-32-28

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The REPL parses the {} as representing a block, not an object literal. Within the if statement's expression there's no such ambiguity, so it works just fine.

}

export function optimizeGroupPlayer(players: AnimationPlayer[]): AnimationPlayer {
Expand Down