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

Speed up calling elm-format by not using npx #49

Merged
merged 2 commits into from
May 30, 2021
Merged

Speed up calling elm-format by not using npx #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b74169
Speed up calling `elm-format` by not using `npx`
lydell May 29, 2021
37921c3
Update lib/autofix.js
jfmengels May 30, 2021
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
141 changes: 90 additions & 51 deletions lib/autofix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const fs = require('fs-extra');
const chalk = require('chalk');
const prompts = require('prompts');
const spawn = require('cross-spawn');
const getPathKey = require('path-key');
const AppState = require('./state');
const ErrorMessage = require('./error-message');
const promisifyPort = require('./promisify-port');
Expand Down Expand Up @@ -103,47 +104,33 @@ function checkIfAFixConfirmationIsStillExpected(app) {
}

function formatFileContent(options, file, filePath) {
if (options.elmFormatPath) {
const spawnedUsingPathFromArgs = spawn.sync(
options.elmFormatPath,
['--elm-version=0.19', '--stdin', '--output', filePath],
{
input: file.source
}
);
const hasElmFormatPathFlag = Boolean(options.elmFormatPath);
const elmFormatPath = options.elmFormatPath || 'elm-format';

const result = spawn.sync(
elmFormatPath,
['--elm-version=0.19', '--stdin', '--output', filePath],
{
input: file.source,
env: hasElmFormatPathFlag
? process.env
: backwardsCompatibleElmFormatEnv()
}
);

if (spawnedUsingPathFromArgs.status !== 0) {
const error = spawnedUsingPathFromArgs.stderr.toString();
if (error.includes('not found')) {
if (result.error) {
if (result.error.code === 'ENOENT') {
if (hasElmFormatPathFlag) {
throw new ErrorMessage.CustomError(
/* eslint-disable prettier/prettier */
'ELM-FORMAT NOT FOUND',
`I could not find the executable for ${chalk.magentaBright('elm-format')} at the location you specified:

${options.elmFormatPath}`,
options.elmJsonPath
/* eslint-enable prettier/prettier */
);
}
}
} else {
const spawnedUsingNpx = spawn.sync(
'npx',
[
'--no-install',
'elm-format',
'--elm-version=0.19',
'--stdin',
'--output',
filePath
],
{
input: file.source
}
);

if (spawnedUsingNpx.status !== 0) {
const error = spawnedUsingNpx.stderr.toString();
if (error.includes('not found')) {
} else {
throw new ErrorMessage.CustomError(
/* eslint-disable prettier/prettier */
'ELM-FORMAT NOT FOUND',
Expand All @@ -157,27 +144,21 @@ A few options:
/* eslint-enable prettier/prettier */
);
}
} else {
throw result.error;
}
}

const spawnedUsingGlobal = spawn.sync(
'elm-format',
['--yes', '--elm-version=0.19', '--stdin', '--output', filePath],
{
input: file.source
}
);

if (spawnedUsingGlobal.status !== 0) {
throw new ErrorMessage.CustomError(
/* eslint-disable prettier/prettier */
'ERROR WHEN RUNNING ELM-FORMAT',
`I got an unexpected error when running ${chalk.magentaBright('elm-format')}:
if (result.status !== 0) {
throw new ErrorMessage.CustomError(
/* eslint-disable prettier/prettier */
'ERROR WHEN RUNNING ELM-FORMAT',
`I got an unexpected error when running ${chalk.magentaBright('elm-format')}:

${spawnedUsingGlobal.stderr.toString()}`,
options.elmJsonPath
/* eslint-enable prettier/prettier */
);
}
}
${result.stderr.toString()}`,
options.elmJsonPath
/* eslint-enable prettier/prettier */
);
}

return {
Expand All @@ -187,3 +168,61 @@ A few options:
source: fs.readFileSync(filePath, 'utf8')
};
}

// When `--elm-format-path` was _not_ provided, we used to execute
// `elm-format` like this:
//
// 1. Try `npx elm-format`
// 2. Try `elm-format`
//
// Just starting `npx` takes 200 ms though. Luckily `npx` isn’t even
// necessary, because the common ways of running `elm-review` are:
//
// 1. Install everything globally and run just `elm-review`.
// 2. Install everything locally and run `npx elm-review`.
// 3. Use the `--elm-format-path`.
//
// That’s also the only supported ways we have for the `elm` binary – there we
// have never tried to execute `npx elm`.
//
// `npx` adds all potential `node_modules/.bin` up to current directory to the
// beginning of PATH, for example:
//
// ❯ npx node -p 'process.env.PATH.split(require("path").delimiter)'
// [
// '/Users/you/stuff/node_modules/.bin',
// '/Users/you/node_modules/.bin',
// '/Users/node_modules/.bin',
// '/node_modules/.bin',
// '/usr/bin',
// 'etc'
// ]
//
// So if a user runs `npx elm-review`, when we later try to spawn just
// `elm-format`, it’ll be found since when spawning we inherit the same PATH.
//
// The `npx elm-format` approach has been removed to avoid those unnessary 200 ms,
// but to stay backwards compatible we prepend the same paths to the beginning
// of PATH just like `npx` would (see above). This is needed when:
//
// - Executing `elm-review` _without_ `npx`.
// - And expecting a _local_ `elm-format` to be used. That’s an odd use case,
// but was supported due to the `npx` approach.
//
// This can be removed in a major version.
function backwardsCompatibleElmFormatEnv() {
const pathKey = getPathKey();
return {
...process.env,
[pathKey]: [
...process
.cwd()
.split(path.sep)
.map((_, index, parts) =>
[...parts.slice(0, index + 1), 'node_modules', '.bin'].join(path.sep)
)
.reverse(),
process.env[pathKey]
].join(path.delimiter)
};
}
Loading