Create setAlmostImmediate, and enforce it via eslint rule

This commit is contained in:
Sergio Padrino 2021-08-12 12:55:08 +02:00
parent 24d89a8007
commit e5bc189793
11 changed files with 257 additions and 13 deletions

View file

@ -25,6 +25,7 @@ rules:
react-no-unbound-dispatcher-props: error
react-readonly-props-and-state: error
react-proper-lifecycle-methods: error
set-almost-immediate: error
###########
# PLUGINS #

View file

@ -132,3 +132,8 @@ export function enableLineChangesInCommit(): boolean {
export function enableWindowsOpenSSH(): boolean {
return enableBetaFeatures()
}
/** Should we use the setImmediate alternative? */
export function enableSetAlmostImmediate(): boolean {
return enableBetaFeatures()
}

View file

@ -0,0 +1,35 @@
/* eslint-disable set-almost-immediate */
import { enableSetAlmostImmediate } from './feature-flag'
/**
* Reference created by setAlmostImmediate so that it can be cleared later.
* It can be NodeJS.Immediate or NodeJS.Timeout because we use a feature flag
* to tweak the behavior of setAlmostImmediate, but this type should be used
* as if it were opaque.
*/
export type AlmostImmediate = NodeJS.Immediate | NodeJS.Timeout
/**
* This function behaves almost like setImmediate, but it will rely on
* setTimeout(..., 0) to actually execute the callback. The reason for this
* is a bug in Electron sometimes causing setImmediate callbacks to not being
* executed.
* For more info about this: https://github.com/electron/electron/issues/29261
*/
export function setAlmostImmediate(
callback: (...args: any[]) => void,
...args: any[]
): AlmostImmediate {
return enableSetAlmostImmediate()
? setTimeout(callback, 0, ...args)
: setImmediate(callback, ...args)
}
/** Used to clear references created by setAlmostImmediate. */
export function clearAlmostImmediate(almostImmediate: AlmostImmediate) {
if (almostImmediate instanceof NodeJS.Immediate) {
clearImmediate(almostImmediate)
} else {
clearTimeout(almostImmediate)
}
}

View file

@ -12,6 +12,7 @@ import {
APICheckConclusion,
} from '../api'
import { IDisposable, Disposable } from 'event-kit'
import { setAlmostImmediate } from '../set-almost-immediate'
/**
* A Desktop-specific model closely related to a GitHub API Check Run.
@ -225,7 +226,7 @@ export class CommitStatusStore {
private queueRefresh() {
if (!this.refreshQueued) {
this.refreshQueued = true
setImmediate(() => {
setAlmostImmediate(() => {
this.refreshQueued = false
this.refreshEligibleSubscriptions()
})

View file

@ -23,6 +23,7 @@ import { clearTagsToPush } from './helpers/tags-to-push-storage'
import { IMatchedGitHubRepository } from '../repository-matching'
import { shallowEquals } from '../equality'
import { enableRepositoryAliases } from '../feature-flag'
import { setAlmostImmediate } from '../set-almost-immediate'
/** The store for local repositories. */
export class RepositoriesStore extends TypedBaseStore<
@ -663,7 +664,7 @@ export class RepositoriesStore extends TypedBaseStore<
*/
private emitUpdatedRepositories() {
if (!this.emitQueued) {
setImmediate(() => {
setAlmostImmediate(() => {
this.getAll()
.then(repos => this.emitUpdate(repos))
.catch(e => log.error(`Failed emitting update`, e))

View file

@ -146,6 +146,7 @@ import {
MultiCommitOperationKind,
MultiCommitOperationStepKind,
} from '../models/multi-commit-operation'
import { setAlmostImmediate } from '../lib/set-almost-immediate'
const MinuteInMilliseconds = 1000 * 60
const HourInMilliseconds = MinuteInMilliseconds * 60
@ -546,7 +547,7 @@ export class App extends React.Component<IAppProps, IAppState> {
}
private boomtown() {
setImmediate(() => {
setAlmostImmediate(() => {
throw new Error('Boomtown!')
})
}

View file

@ -8,6 +8,11 @@ import { OnionSkin } from './onion-skin'
import { Swipe } from './swipe'
import { assertNever } from '../../../lib/fatal-error'
import { ISize, getMaxFitSize } from './sizing'
import {
AlmostImmediate,
clearAlmostImmediate,
setAlmostImmediate,
} from '../../../lib/set-almost-immediate'
interface IModifiedImageDiffProps {
readonly previous: Image
@ -63,7 +68,7 @@ export class ModifiedImageDiff extends React.Component<
private container: HTMLElement | null = null
private readonly resizeObserver: ResizeObserver
private resizedTimeoutID: NodeJS.Immediate | null = null
private resizedTimeoutID: AlmostImmediate | null = null
public constructor(props: IModifiedImageDiffProps) {
super(props)
@ -75,10 +80,10 @@ export class ModifiedImageDiff extends React.Component<
// when we're reacting to a resize so we'll defer it until after
// react is done with this frame.
if (this.resizedTimeoutID !== null) {
clearImmediate(this.resizedTimeoutID)
clearAlmostImmediate(this.resizedTimeoutID)
}
this.resizedTimeoutID = setImmediate(
this.resizedTimeoutID = setAlmostImmediate(
this.onResized,
entry.target,
entry.contentRect

View file

@ -14,6 +14,11 @@ import { wrapRichTextCommitMessage } from '../../lib/wrap-rich-text-commit-messa
import { DiffOptions } from '../diff/diff-options'
import { RepositorySectionTab } from '../../lib/app-state'
import { IChangesetData } from '../../lib/git'
import {
AlmostImmediate,
clearAlmostImmediate,
setAlmostImmediate,
} from '../../lib/set-almost-immediate'
interface ICommitSummaryProps {
readonly repository: Repository
@ -124,7 +129,7 @@ export class CommitSummary extends React.Component<
> {
private descriptionScrollViewRef: HTMLDivElement | null = null
private readonly resizeObserver: ResizeObserver | null = null
private updateOverflowTimeoutId: NodeJS.Immediate | null = null
private updateOverflowTimeoutId: AlmostImmediate | null = null
private descriptionRef: HTMLDivElement | null = null
public constructor(props: ICommitSummaryProps) {
@ -143,10 +148,10 @@ export class CommitSummary extends React.Component<
// when we're reacting to a resize so we'll defer it until after
// react is done with this frame.
if (this.updateOverflowTimeoutId !== null) {
clearImmediate(this.updateOverflowTimeoutId)
clearAlmostImmediate(this.updateOverflowTimeoutId)
}
this.updateOverflowTimeoutId = setImmediate(this.onResized)
this.updateOverflowTimeoutId = setAlmostImmediate(this.onResized)
}
}
})

View file

@ -17,6 +17,11 @@ import { createUniqueId, releaseUniqueId } from '../../lib/id-pool'
import { range } from '../../../lib/range'
import { ListItemInsertionOverlay } from './list-item-insertion-overlay'
import { DragData, DragType } from '../../../models/drag-drop'
import {
AlmostImmediate,
clearAlmostImmediate,
setAlmostImmediate,
} from '../../../lib/set-almost-immediate'
/**
* Describe the first argument given to the cellRenderer,
@ -276,7 +281,7 @@ export class List extends React.Component<IListProps, IListState> {
private list: HTMLDivElement | null = null
private grid: Grid | null = null
private readonly resizeObserver: ResizeObserver | null = null
private updateSizeTimeoutId: NodeJS.Immediate | null = null
private updateSizeTimeoutId: AlmostImmediate | null = null
public constructor(props: IListProps) {
super(props)
@ -294,10 +299,10 @@ export class List extends React.Component<IListProps, IListState> {
// when we're reacting to a resize so we'll defer it until after
// react is done with this frame.
if (this.updateSizeTimeoutId !== null) {
clearImmediate(this.updateSizeTimeoutId)
clearAlmostImmediate(this.updateSizeTimeoutId)
}
this.updateSizeTimeoutId = setImmediate(
this.updateSizeTimeoutId = setAlmostImmediate(
this.onResized,
entry.target,
entry.contentRect
@ -774,7 +779,7 @@ export class List extends React.Component<IListProps, IListState> {
public componentWillUnmount() {
if (this.updateSizeTimeoutId !== null) {
clearImmediate(this.updateSizeTimeoutId)
clearAlmostImmediate(this.updateSizeTimeoutId)
this.updateSizeTimeoutId = null
}

View file

@ -0,0 +1,105 @@
// @ts-check
/**
* set-almost-immediate
*
* This custom eslint rule is highly specific to GitHub Desktop and attempts
* to prevent using setImmediate since, under some circumstances, could not work
* at all in Electron 11.4.0+
*
* For more info about this issue, see: https://github.com/electron/electron/issues/29261
*
* As long as this issue persists, we'll use an alternative named setAlmostImmediate,
* and this rule will ensure that's used instead of setImmediate.
*/
/**
* @typedef {import('@typescript-eslint/experimental-utils').TSESLint.RuleModule} RuleModule
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.TSTypeAnnotation} TSTypeAnnotation
* @typedef {import("@typescript-eslint/typescript-estree").TSESTree.TypeNode} TypeNode
*/
/** @type {RuleModule} */
module.exports = {
meta: {
type: 'problem',
messages: {
setImmediateForbidden: `setImmediate cannot be used, use setAlmostImmediate instead`,
clearImmediateForbidden: `clearImmediate cannot be used, use clearAlmostImmediate instead`,
nodeJSImmediateForbidden: `NodeJS.Immediate cannot be used, use AlmostImmediate instead`,
},
fixable: 'code',
schema: [],
},
create: function (context) {
const sourceCode = context.getSourceCode()
/**
* Check each member of the interface body and ensure it is marked `readonly`.
*
* @param {TSTypeAnnotation} node
* @param {TypeNode} typeAnnotation
*/
function scanTypeAnnotation(node, typeAnnotation) {
if (typeAnnotation.type === 'TSTypeReference') {
const typeName = sourceCode.getText(typeAnnotation)
if (typeName === 'NodeJS.Immediate') {
context.report({
loc: typeAnnotation.loc,
messageId: 'nodeJSImmediateForbidden',
fix: fixer => {
return fixer.replaceTextRange(
typeAnnotation.range,
'AlmostImmediate'
)
},
})
}
} else if ('types' in typeAnnotation) {
for (const type of typeAnnotation.types) {
scanTypeAnnotation(node, type)
}
}
}
return {
TSTypeAnnotation(node) {
scanTypeAnnotation(node, node.typeAnnotation)
},
CallExpression(node) {
const { callee } = node
if (callee.type !== 'Identifier') {
return
}
const functionName = sourceCode.getText(callee)
const offendingFunctions = {
setImmediate: {
messageId: 'setImmediateForbidden',
replacement: 'setAlmostImmediate',
},
clearImmediate: {
messageId: 'clearImmediateForbidden',
replacement: 'clearAlmostImmediate',
},
}
const offendingCall = offendingFunctions[functionName]
if (offendingCall !== undefined) {
context.report({
node: callee,
messageId: offendingCall.messageId,
fix: fixer => {
return fixer.replaceTextRange(
node.callee.range,
offendingCall.replacement
)
},
})
}
},
}
},
}

View file

@ -0,0 +1,80 @@
// @ts-check
const { ESLintUtils } = require('@typescript-eslint/experimental-utils')
const RuleTester = ESLintUtils.RuleTester
const rule = require('../set-almost-immediate')
// ------------------------------------------------------------------------------
// Tests
// ------------------------------------------------------------------------------
const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
})
ruleTester.run('set-almost-immediate', rule, {
valid: [
{
filename: 'app/src/ui/diff/helper.ts',
code: `
const ref: AlmostImmediate = setAlmostImmediate(() => {})
clearAlmostImmediate(ref)
`,
},
{
filename: 'app/src/ui/some-class.ts',
code: `
class SomeClass {
private ref: AlmostImmediate
}
`,
},
],
invalid: [
{
filename: 'app/src/ui/helper.ts',
code: `
const ref: NodeJS.Immediate = setImmediate(() => {})
clearImmediate(ref)
`,
errors: [
{
messageId: 'nodeJSImmediateForbidden',
},
{
messageId: 'setImmediateForbidden',
},
{
messageId: 'clearImmediateForbidden',
},
],
output: `
const ref: AlmostImmediate = setAlmostImmediate(() => {})
clearAlmostImmediate(ref)
`,
},
{
filename: 'app/src/ui/some-class.ts',
code: `
class SomeClass {
private ref: NodeJS.Immediate
private nullableRef: NodeJS.Immediate | null
}
`,
errors: [
{
messageId: 'nodeJSImmediateForbidden',
},
{
messageId: 'nodeJSImmediateForbidden',
},
],
output: `
class SomeClass {
private ref: AlmostImmediate
private nullableRef: AlmostImmediate | null
}
`,
},
],
})