Skip to content

Discourage use of 'window' and 'document' #2874

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.

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

Merged
merged 1 commit into from
Apr 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/firestore/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ module.exports = {
varsIgnorePattern: '^_',
args: 'none'
}
],
'no-restricted-globals': [
'error',
{
'name': 'window',
'message': 'Use `PlatformSupport.getPlatform().window` instead.'
},
{
'name': 'document',
'message': 'Use `PlatformSupport.getPlatform().document` instead.'
}
]
},
overrides: [
Expand Down
3 changes: 3 additions & 0 deletions packages/firestore/src/local/simple_db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import { Deferred } from '../util/promise';
import { SCHEMA_VERSION } from './indexeddb_schema';
import { PersistencePromise } from './persistence_promise';

// References to `window` are guarded by SimpleDb.isAvailable()
/* eslint-disable no-restricted-globals */

const LOG_TAG = 'SimpleDb';

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import {
NetworkStatus
} from './../remote/connectivity_monitor';

// References to `window` are guarded by BrowserConnectivityMonitor.isAvailable()
/* eslint-disable no-restricted-globals */

const LOG_TAG = 'ConnectivityMonitor';

/**
Expand Down
7 changes: 6 additions & 1 deletion packages/firestore/src/platform_browser/browser_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ import { Platform } from '../platform/platform';
import { Connection } from '../remote/connection';
import { JsonProtoSerializer } from '../remote/serializer';
import { ConnectivityMonitor } from './../remote/connectivity_monitor';

import { NoopConnectivityMonitor } from '../remote/connectivity_monitor_noop';
import { BrowserConnectivityMonitor } from './browser_connectivity_monitor';
import { WebChannelConnection } from './webchannel_connection';

// Implements the Platform API for browsers and some browser-like environments
// (including ReactNative).
export class BrowserPlatform implements Platform {
readonly useProto3Json = true;
readonly base64Available: boolean;
Expand All @@ -34,10 +35,14 @@ export class BrowserPlatform implements Platform {
}

get document(): Document | null {
// `document` is not always available, e.g. in ReactNative and WebWorkers.
// eslint-disable-next-line no-restricted-globals
return typeof document !== 'undefined' ? document : null;
}

get window(): Window | null {
// `window` is not always available, e.g. in ReactNative and WebWorkers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by comment: should this fall back on self in a web worker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know and we should be careful if we want to make this change. FWIW, this doc says to always use self: https://developer.mozilla.org/en-US/docs/Web/API/Window/self

This works because JS in browsers prefixes any unknown globals with window and window.self seems to be a thing.

// eslint-disable-next-line no-restricted-globals
return typeof window !== 'undefined' ? window : null;
}

Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/platform_node/node_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export class NodePlatform implements Platform {

get window(): Window | null {
if (process.env.USE_MOCK_PERSISTENCE === 'YES') {
// eslint-disable-next-line no-restricted-globals
return window;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import * as api from '../../../src/protos/firestore_proto_api';
import { DEFAULT_PROJECT_ID } from '../util/helpers';
import { getDefaultDatabaseInfo } from '../util/internal_helpers';

/* eslint-disable no-restricted-globals */

// We need to check both `window` and `window.navigator` to make sure we are
// not running in Node with IndexedDBShim.
const describeFn =
Expand Down
3 changes: 3 additions & 0 deletions packages/firestore/test/integration/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@

import * as firestore from '@firebase/firestore-types';
import firebase from './firebase_export';

/* eslint-disable no-restricted-globals */

/**
* NOTE: These helpers are used by api/ tests and therefore may not have any
* dependencies on src/ files.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ import {
TEST_SERIALIZER
} from './persistence_test_helpers';

/* eslint-disable no-restricted-globals */

function withDb(
schemaVersion: number,
fn: (db: IDBDatabase) => Promise<void>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import { PersistencePromise } from '../../../src/local/persistence_promise';

import * as chaiAsPromised from 'chai-as-promised';

/* eslint-disable no-restricted-globals */

use(chaiAsPromised);

describe('PersistencePromise', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ import { AsyncQueue } from '../../../src/util/async_queue';
import { FirestoreError } from '../../../src/util/error';
import { AutoId } from '../../../src/util/misc';

/* eslint-disable no-restricted-globals */

/** The prefix used by the keys that Firestore writes to Local Storage. */
const LOCAL_STORAGE_PREFIX = 'firestore_';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ import {
populateWebStorage
} from './persistence_test_helpers';

/* eslint-disable no-restricted-globals */

const AUTHENTICATED_USER = new User('test');
const UNAUTHENTICATED_USER = User.UNAUTHENTICATED;
const TEST_ERROR = new FirestoreError('internal', 'Test Error');
Expand Down
2 changes: 2 additions & 0 deletions packages/firestore/test/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ import { PlatformSupport } from '../../src/platform/platform';
import { JsonProtoSerializer } from '../../src/remote/serializer';
import { Timestamp } from '../../src/api/timestamp';

/* eslint-disable no-restricted-globals */

export type TestSnapshotVersion = number;

/**
Expand Down
2 changes: 2 additions & 0 deletions packages/firestore/test/util/test_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { assert, fail } from '../../src/util/assert';
import { ConnectivityMonitor } from './../../src/remote/connectivity_monitor';
import { NoopConnectivityMonitor } from './../../src/remote/connectivity_monitor_noop';

/* eslint-disable no-restricted-globals */

/**
* `Window` fake that implements the event and storage API that is used by
* Firestore.
Expand Down