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.

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

refactor: allow requiring modules with no side effects #17496

Merged
merged 1 commit into from
Apr 30, 2019
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
9 changes: 5 additions & 4 deletions atom/browser/api/atom_api_power_monitor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,10 @@ int PowerMonitor::GetSystemIdleTime() {
// static
v8::Local<v8::Value> PowerMonitor::Create(v8::Isolate* isolate) {
if (!Browser::Get()->is_ready()) {
isolate->ThrowException(v8::Exception::Error(mate::StringToV8(
isolate,
"Cannot require \"powerMonitor\" module before app is ready")));
isolate->ThrowException(v8::Exception::Error(
mate::StringToV8(isolate,
"The 'powerMonitor' module can't be used before the "
"app 'ready' event")));
return v8::Null(isolate);
}

Expand Down Expand Up @@ -139,7 +140,7 @@ void Initialize(v8::Local<v8::Object> exports,
void* priv) {
v8::Isolate* isolate = context->GetIsolate();
mate::Dictionary dict(isolate, exports);
dict.Set("powerMonitor", PowerMonitor::Create(isolate));
dict.Set("createPowerMonitor", base::Bind(&PowerMonitor::Create, isolate));
dict.Set("PowerMonitor", PowerMonitor::GetConstructor(isolate)
->GetFunction(context)
.ToLocalChecked());
Expand Down
5 changes: 3 additions & 2 deletions atom/browser/api/atom_api_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ void Screen::OnDisplayMetricsChanged(const display::Display& display,
v8::Local<v8::Value> Screen::Create(v8::Isolate* isolate) {
if (!Browser::Get()->is_ready()) {
isolate->ThrowException(v8::Exception::Error(mate::StringToV8(
isolate, "Cannot require \"screen\" module before app is ready")));
isolate,
"The 'screen' module can't be used before the app 'ready' event")));
return v8::Null(isolate);
}

Expand Down Expand Up @@ -162,7 +163,7 @@ void Initialize(v8::Local<v8::Object> exports,
void* priv) {
v8::Isolate* isolate = context->GetIsolate();
mate::Dictionary dict(isolate, exports);
dict.Set("screen", Screen::Create(isolate));
dict.Set("createScreen", base::Bind(&Screen::Create, isolate));
dict.Set(
"Screen",
Screen::GetConstructor(isolate)->GetFunction(context).ToLocalChecked());
Expand Down
17 changes: 8 additions & 9 deletions docs/api/power-monitor.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@

Process: [Main](../glossary.md#main-process)

You cannot require or use this module until the `ready` event of the `app`

This module cannot be used until the `ready` event of the `app`
module is emitted.

For example:

```javascript
const electron = require('electron')
const { app } = electron
const { app, powerMonitor } = require('electron')

app.on('ready', () => {
electron.powerMonitor.on('suspend', () => {
powerMonitor.on('suspend', () => {
console.log('The system is going to sleep')
})
})
Expand Down Expand Up @@ -59,7 +59,7 @@ Emitted as soon as the systems screen is unlocked.

The `powerMonitor` module has the following methods:

#### `powerMonitor.querySystemIdleState(idleThreshold, callback)` _(Deprecated)_
### `powerMonitor.querySystemIdleState(idleThreshold, callback)` _(Deprecated)_

* `idleThreshold` Integer
* `callback` Function
Expand All @@ -70,14 +70,14 @@ before considered idle. `callback` will be called synchronously on some systems
and with an `idleState` argument that describes the system's state. `locked` is
available on supported systems only.

#### `powerMonitor.querySystemIdleTime(callback)` _(Deprecated)_
### `powerMonitor.querySystemIdleTime(callback)` _(Deprecated)_

* `callback` Function
* `idleTime` Integer - Idle time in seconds

Calculate system idle time in seconds.

#### `powerMonitor.getSystemIdleState(idleThreshold)`
### `powerMonitor.getSystemIdleState(idleThreshold)`

* `idleThreshold` Integer

Expand All @@ -86,9 +86,8 @@ Returns `String` - The system's current state. Can be `active`, `idle`, `locked`
Calculate the system idle state. `idleThreshold` is the amount of time (in seconds)
before considered idle. `locked` is available on supported systems only.

#### `powerMonitor.getSystemIdleTime()`
### `powerMonitor.getSystemIdleTime()`

Returns `Integer` - Idle time in seconds

Calculate system idle time in seconds.

13 changes: 5 additions & 8 deletions docs/api/screen.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

Process: [Main](../glossary.md#main-process)

You cannot require or use this module until the `ready` event of the `app`
This module cannot be used until the `ready` event of the `app`
module is emitted.

`screen` is an [EventEmitter](https://nodejs.org/api/events.html#events_class_eventemitter).
Expand All @@ -15,13 +15,11 @@ property, so writing `let { screen } = require('electron')` will not work.
An example of creating a window that fills the whole screen:

```javascript
const electron = require('electron')
const { app, BrowserWindow } = electron
const { app, BrowserWindow, screen } = require('electron')

let win

app.on('ready', () => {
const { width, height } = electron.screen.getPrimaryDisplay().workAreaSize
const { width, height } = screen.getPrimaryDisplay().workAreaSize
win = new BrowserWindow({ width, height })
win.loadURL('https://github.com')
})
Expand All @@ -30,13 +28,12 @@ app.on('ready', () => {
Another example of creating a window in the external display:

```javascript
const electron = require('electron')
const { app, BrowserWindow } = require('electron')
const { app, BrowserWindow, screen } = require('electron')

let win

app.on('ready', () => {
let displays = electron.screen.getAllDisplays()
let displays = screen.getAllDisplays()
let externalDisplay = displays.find((display) => {
return display.bounds.x !== 0 || display.bounds.y !== 0
})
Expand Down
5 changes: 3 additions & 2 deletions filenames.gni
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ filenames = {
"lib/browser/api/net.js",
"lib/browser/api/net-log.js",
"lib/browser/api/notification.js",
"lib/browser/api/power-monitor.js",
"lib/browser/api/power-monitor.ts",
"lib/browser/api/power-save-blocker.js",
"lib/browser/api/protocol.ts",
"lib/browser/api/screen.js",
"lib/browser/api/screen.ts",
"lib/browser/api/session.js",
"lib/browser/api/system-preferences.js",
"lib/browser/api/top-level-window.js",
Expand All @@ -46,6 +46,7 @@ filenames = {
"lib/browser/navigation-controller.js",
"lib/browser/objects-registry.js",
"lib/browser/rpc-server.js",
"lib/browser/utils.ts",
"lib/common/api/clipboard.js",
"lib/common/api/deprecate.ts",
"lib/common/api/deprecations.js",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,53 +1,47 @@
'use strict'

import { createLazyInstance } from '../utils'

const { EventEmitter } = require('events')
const { powerMonitor, PowerMonitor } = process.electronBinding('power_monitor')
const { createPowerMonitor, PowerMonitor } = process.electronBinding('power_monitor')
const { deprecate } = require('electron')

// PowerMonitor is an EventEmitter.
Object.setPrototypeOf(PowerMonitor.prototype, EventEmitter.prototype)
EventEmitter.call(powerMonitor)

const powerMonitor = createLazyInstance(createPowerMonitor, PowerMonitor, true)

// On Linux we need to call blockShutdown() to subscribe to shutdown event.
if (process.platform === 'linux') {
powerMonitor.on('newListener', (event) => {
powerMonitor.on('newListener', (event:string) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {
powerMonitor.blockShutdown()
}
})

powerMonitor.on('removeListener', (event) => {
powerMonitor.on('removeListener', (event: string) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {
powerMonitor.unblockShutdown()
}
})
}

// TODO(nitsakh): Remove in 7.0
powerMonitor.querySystemIdleState = function (threshold, callback) {
powerMonitor.querySystemIdleState = function (threshold: number, callback: Function) {
codebytere marked this conversation as resolved.
Show resolved Hide resolved
deprecate.warn('powerMonitor.querySystemIdleState', 'powerMonitor.getSystemIdleState')
if (typeof threshold !== 'number') {
throw new Error('Must pass threshold as a number')
}

if (typeof callback !== 'function') {
throw new Error('Must pass callback as a function argument')
}
if (typeof threshold !== 'number') throw new Error('Must pass threshold as a number')
if (typeof callback !== 'function') throw new Error('Must pass callback as a function argument')

const idleState = this.getSystemIdleState(threshold)

process.nextTick(() => callback(idleState))
}

// TODO(nitsakh): Remove in 7.0
powerMonitor.querySystemIdleTime = function (callback) {
powerMonitor.querySystemIdleTime = function (callback: Function) {
codebytere marked this conversation as resolved.
Show resolved Hide resolved
deprecate.warn('powerMonitor.querySystemIdleTime', 'powerMonitor.getSystemIdleTime')
if (typeof callback !== 'function') {
throw new Error('Must pass function as an argument')
}
if (typeof callback !== 'function') throw new Error('Must pass function as an argument')

const idleTime = this.getSystemIdleTime()

process.nextTick(() => callback(idleTime))
}

Expand Down
10 changes: 0 additions & 10 deletions lib/browser/api/screen.js

This file was deleted.

10 changes: 10 additions & 0 deletions lib/browser/api/screen.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
'use strict'

import { createLazyInstance } from '../utils'
const { EventEmitter } = require('events')
const { Screen, createScreen } = process.electronBinding('screen')

// Screen is an EventEmitter.
Object.setPrototypeOf(Screen.prototype, EventEmitter.prototype)

module.exports = createLazyInstance(createScreen, Screen, true)
38 changes: 38 additions & 0 deletions lib/browser/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { EventEmitter } from 'events'

/**
* Creates a lazy instance of modules that can't be required before the
* app 'ready' event by returning a proxy object to mitigate side effects
* on 'require'
*
* @param {Function} creator - Function that creates a new module instance
* @param {Object} holder - the object holding the module prototype
* @param {Boolean} isEventEmitter - whether or not the module is an EventEmitter
codebytere marked this conversation as resolved.
Show resolved Hide resolved
* @returns {Object} - a proxy object for the
*/

export function createLazyInstance (
Copy link
Contributor

@miniak miniak Apr 9, 2019

Choose a reason for hiding this comment

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

could we maybe use an ES6 Proxy?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably not unfortunately, the intricacies of doing that correctly would introduce far more space for potential issues

Copy link
Contributor

Choose a reason for hiding this comment

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

The remote module does not work very well with ES6 Proxy objects.

creator: Function,
holder: Object,
isEventEmitter: Boolean
codebytere marked this conversation as resolved.
Show resolved Hide resolved
) {
let lazyModule: Object
const module: any = {}
for (const method in (holder as any).prototype) {
module[method] = (...args: any) => {
codebytere marked this conversation as resolved.
Show resolved Hide resolved
// create new instance of module at runtime if none exists
if (!lazyModule) {
lazyModule = creator()
if (isEventEmitter) EventEmitter.call(lazyModule as any)
}

// check for properties on the prototype chain that aren't functions
if (typeof (lazyModule as any)[method] !== 'function') {
return (lazyModule as any)[method]
}

return (lazyModule as any)[method](...args)
}
}
return module
}
2 changes: 1 addition & 1 deletion spec-main/events-helpers.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { EventEmitter } from "electron";
import { EventEmitter } from 'electron';

/**
* @fileoverview A set of helper functions to make it easier to work
Expand Down