Skip to content
Permalink
Browse files

Fix: Stability issues with VS Code extension

The VS Code extension could end up breaking `node_modules` in the
workspace or the global shared folder. This was caused because we were
installing `hint@latest` in the workspace and because that process
took too long and the user might closed the editor before it was
ended.
To avoid that we now let the user manage their own `hint` installation
in the workspace, no more prompting about installation. If no version
can be loaded from the workspace, the shared one (`hint@latest`) will
be used.
Also, the shared version is updated after 2 minutes instead of
immediately and only if the shared version has been successfully
loaded.
If the user already has a workspace version there is no point in
updating the shared one as it will not be used.
  • Loading branch information
molant committed Nov 18, 2019
1 parent afa82f7 commit 7e77a1349e5e2e7f90da75ad1380af32e6838acc
@@ -103,6 +103,7 @@ export const trackSave = (uri: string, languageId: string) => {
}

// Throttle tracking saves to reduce redundant reports when autosave is on.
/* istanbul ignore if */
if (lastSave && now - lastSave < twoMinutes) {
return;
}
@@ -4,9 +4,9 @@ import { TextDocument, PublishDiagnosticsParams, Connection } from 'vscode-langu
import { trackResult } from './analytics';
import { getUserConfig } from './config';
import * as notifications from './notifications';
import { loadWebhint, updateSharedWebhint } from './webhint-packages';
import { loadWebhint } from './webhint-packages';
import { problemToDiagnostic } from './problems';
import { promptAddWebhint, promptRetry } from './prompts';
import { promptRetry } from './prompts';

const analyze = async (textDocument: TextDocument, webhint: import('hint').Analyzer): Promise<PublishDiagnosticsParams> => {
const { languageId, uri } = textDocument;
@@ -35,52 +35,38 @@ export class Analyzer {
private loaded = false;
private validating = false;
private validationQueue: TextDocument[] = [];
private didUpdateSharedWebhint = false;
private webhint: import('hint').Analyzer | null = null;

public constructor(_globalStoragePath: string, _connection: Connection) {
this.connection = _connection;
this.globalStoragePath = _globalStoragePath;
}

// Load both webhint and a configuration, adjusting it as needed for this extension.
private async initWebhint(directory: string): Promise<import('hint').Analyzer | null> {
const hintModule = await loadWebhint(directory, this.globalStoragePath, (install) => {
return promptAddWebhint(this.connection.window, async () => {
this.connection.sendNotification(notifications.showOutput);
await install();
this.onConfigurationChanged();
});
});
/**
* Load both webhint and a configuration, adjusting it as needed for this extension.
* If `directory` is not passed, the shared installation will be used.
*/
private async initWebhint(directory = ''): Promise<import('hint').Analyzer | null> {
const hintModule = await loadWebhint(directory, this.globalStoragePath);

// If no module was returned, the user cancelled installing webhint.
/* istanbul ignore if */
if (!hintModule) {
return null;
}

const userConfig = getUserConfig(hintModule, directory);

try {
const webhint = hintModule.createAnalyzer(userConfig);

// After first load, ensure shared copy of webhint is up-to-date for next use.
if (!this.didUpdateSharedWebhint) {
this.didUpdateSharedWebhint = true;
updateSharedWebhint(this.globalStoragePath); // Does not `await` to avoid delaying startup.
}

return webhint;
return hintModule.createAnalyzer(userConfig);
} catch (e) {
// Instantiating webhint failed, log the error to the webhint output panel to aid debugging.
console.error(e);

return await promptRetry(this.connection.window, async () => {
return await promptRetry(this.connection.window, /* istanbul ignore next */ () => {
this.connection.sendNotification(notifications.showOutput);

// Ensure shared instance is up-to-date before retrying.
await updateSharedWebhint(this.globalStoragePath);

return this.initWebhint(directory);
// We retry with the shared version as it is more likely to not be broken 🀞
return this.initWebhint();
});
}
}
@@ -1,24 +1,5 @@
import { RemoteWindow } from 'vscode-languageserver';

export const promptAddWebhint = async (window: RemoteWindow, install: () => Promise<void>) => {
const addWebhint = 'Add webhint';
const cancel = 'Cancel';
const answer = await window.showInformationMessage(
'A local `.hintrc` was found. Add webhint to this project?',
{ title: addWebhint },
{ title: cancel }
);

if (answer && answer.title === addWebhint) {
try {
await install();
window.showInformationMessage('Finished installing webhint!');
} catch (err) {
window.showErrorMessage(`Unable to install webhint:\n${err}`);
}
}
};

export const promptRetry = async <T>(window: RemoteWindow, retry: () => Promise<T | null>): Promise<T | null> => {
// Prompt the user to retry after checking their configuration.
const retryTitle = 'Retry';
@@ -1,6 +1,9 @@
import { hasFile, mkdir } from './fs';
import { createPackageJson, installPackages, loadPackage, InstallOptions } from './packages';

/** Timeout to start updating the shared webhint install. */
const updateWebhintTimeout = 120000;

/* istanbul ignore next */
const installWebhint = (options: InstallOptions) => {
return installPackages(['hint@latest', 'typescript@latest'], options);
@@ -39,9 +42,27 @@ export const updateSharedWebhint = async (globalStoragePath: string) => {
/* istanbul ignore next */
const loadSharedWebhint = async (globalStoragePath: string): Promise<typeof import('hint') | null> => {
try {
return loadPackage('hint', { paths: [globalStoragePath] });
const hintPkg = await loadPackage('hint', { paths: [globalStoragePath] }) as typeof import('hint');

/**
* The shared package has been loaded successfully but it could be outdated.
* The update process kicks off after a few seconds to allow everything to
* get started first.
*/
setTimeout(() => {
console.log(`Updating shared version of "hint"`);

updateSharedWebhint(globalStoragePath);
}, updateWebhintTimeout);

return hintPkg;
} catch (e) {
try {
console.error(`Error loading shared "hint" package`);
console.error(e);

console.log(`Installing shared version of "hint"`);

await updateSharedWebhint(globalStoragePath);

return loadPackage('hint', { paths: [globalStoragePath] });
@@ -54,24 +75,32 @@ const loadSharedWebhint = async (globalStoragePath: string): Promise<typeof impo
};

/**
* Load webhint, installing it if needed.
* Will prompt to install a local copy if `.hintrc` is present.
* Tries to load webhint from the user workspace, then the shared copy
* unless `directory` is an empty string. In that cases it loads the
* shared global version directly.
*/
export const loadWebhint = async (directory: string, globalStoragePath: string, promptToInstall: (install: () => Promise<void>) => Promise<void>): Promise<typeof import('hint') | null> => {
export const loadWebhint = (directory: string, globalStoragePath: string): Promise<typeof import('hint') | null> => {
try {
return loadPackage('hint', { paths: [directory] });
} catch (e) {
if (await hasFile('.hintrc', directory)) {
/**
* Prompt to install, but don't wait in case the user ignores.
* Load the shared copy for now until the install is done.
* Caller is expected to reload once install is complete.
*/
promptToInstall(async () => {
await installWebhint({ cwd: directory });
});
/* istanbul ignore else */
if (directory) {
return loadPackage('hint', { paths: [directory] });
}

/* istanbul ignore next */
return loadSharedWebhint(globalStoragePath);
} catch (e) /* istanbul ignore next */ {
console.error(`Error loading "hint" package from "${directory}"`);

/**
* If `directory` exists, we tried to load the workspace version first and failed
* so we try again with the shared one now.
*/
if (directory) {
console.error(`Trying to load shared version`);

return loadSharedWebhint(globalStoragePath);
}

return Promise.resolve(null);
}
};
@@ -62,15 +62,15 @@ test('It translates missing endColumn and endLine properties correctly', (t) =>
hintId: 'test-id-1',
location,
message: 'Test Message 1',
severity: Severity.warning
severity: Severity.hint
} as Problem;

const diagnostic = problemToDiagnostic(problem);

t.is(diagnostic.source, 'webhint');
t.true(diagnostic.message.indexOf(problem.message || '') !== -1);
t.true(diagnostic.message.indexOf(problem.hintId || '') !== -1);
t.is(diagnostic.severity, DiagnosticSeverity.Warning);
t.is(diagnostic.severity, DiagnosticSeverity.Hint);
t.is(diagnostic.range.start.line, location.line);
t.is(diagnostic.range.start.character, location.column);
t.is(diagnostic.range.end.character, location.column);
@@ -2,77 +2,7 @@ import test from 'ava';
import * as sinon from 'sinon';
import { RemoteWindow } from 'vscode-languageserver';

import { promptAddWebhint, promptRetry } from '../../src/utils/prompts';

test('It prompts the user to install webhint', async (t) => {
const install = sinon.stub().resolves();
const showInformationMessage = sinon
.stub<Parameters<RemoteWindow['showInformationMessage']>>()
.resolves();

await promptAddWebhint(
{ showInformationMessage } as Partial<RemoteWindow> as RemoteWindow,
install
);

t.is(install.callCount, 0);
t.is(showInformationMessage.callCount, 1);
t.is(showInformationMessage.firstCall.args.length, 3);
t.is(showInformationMessage.firstCall.args[0], 'A local `.hintrc` was found. Add webhint to this project?');
t.deepEqual(showInformationMessage.firstCall.args[1], { title: 'Add webhint'});
t.deepEqual(showInformationMessage.firstCall.args[2], { title: 'Cancel'});
});

test('It does not install if the user selects "Cancel"', async (t) => {
const install = sinon.stub().resolves();
const showInformationMessage = sinon
.stub<Parameters<RemoteWindow['showInformationMessage']>>()
.resolves({ title: 'Cancel '});

await promptAddWebhint(
{ showInformationMessage } as Partial<RemoteWindow> as RemoteWindow,
install
);

t.is(install.callCount, 0);
});

test('It does install if the user selects "Add webhint"', async (t) => {
const install = sinon.stub().resolves();
const showInformationMessage = sinon
.stub<Parameters<RemoteWindow['showInformationMessage']>>()
.resolves({ title: 'Add webhint' });

await promptAddWebhint(
{ showInformationMessage } as Partial<RemoteWindow> as RemoteWindow,
install
);

t.is(install.callCount, 1);
t.is(showInformationMessage.callCount, 2);
t.is(showInformationMessage.secondCall.args.length, 1);
t.is(showInformationMessage.secondCall.args[0], 'Finished installing webhint!');
});

test('It notifies the user if installation fails', async (t) => {
const install = sinon.stub().throws();
const showErrorMessage = sinon
.stub<Parameters<RemoteWindow['showErrorMessage']>>()
.resolves();
const showInformationMessage = sinon
.stub<Parameters<RemoteWindow['showInformationMessage']>>()
.resolves({ title: 'Add webhint' });

await promptAddWebhint(
{ showErrorMessage, showInformationMessage } as Partial<RemoteWindow> as RemoteWindow,
install
);

t.is(install.callCount, 1);
t.is(showErrorMessage.callCount, 1);
t.is(showErrorMessage.firstCall.args.length, 1);
t.regex(showErrorMessage.firstCall.args[0], /^Unable to install webhint/);
});
import { promptRetry } from '../../src/utils/prompts';

test('It prompts the user to retry if initializing webhint fails', async (t) => {
const retry = sinon.stub().resolves(null);
@@ -64,22 +64,9 @@ test('It loads shared webhint when prompting to install a local copy', async (t)
const { module, stubs } = stubContext();
const loadPackageSpy = sandbox.spy(stubs['./packages'], 'loadPackage');

let promptCalled = false;
let promptComplete = false;

const hint = await module.loadWebhint('local', 'global', async () => {
promptCalled = true;

await new Promise((resolve) => {
setTimeout(resolve, 0);
});

promptComplete = true;
});
const hint = await module.loadWebhint('local', 'global');

t.is(hint as any, 'webhint');
t.true(promptCalled);
t.false(promptComplete);
t.true(loadPackageSpy.calledTwice);
t.deepEqual(loadPackageSpy.firstCall.args[0], 'hint');
t.deepEqual(loadPackageSpy.firstCall.args[1], { paths: ['local'] });

0 comments on commit 7e77a13

Please sign in to comment.
You can’t perform that action at this time.