Skip to content

Commit

Permalink
refactor: npx replacement
Browse files Browse the repository at this point in the history
Closes #224.
  • Loading branch information
lishaduck committed Aug 30, 2024
1 parent 20bd482 commit 3af193d
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 83 deletions.
62 changes: 2 additions & 60 deletions lib/autofix.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ const path = require('node:path');
const chalk = require('chalk');
const prompts = require('prompts');
const spawn = require('cross-spawn');
const getPathKey = require('path-key');
const FS = require('./fs-wrapper');
const AppState = require('./state');
const ErrorMessage = require('./error-message');
Expand All @@ -12,6 +11,7 @@ const StyledMessage = require('./styled-message');
const {startReview} = require('./run-review');
const ProjectDependencies = require('./project-dependencies');
const exit = require('../vendor/exit');
const {backwardsCompatiblePath, pathKey} = require('./npx');

/**
* @import {App, AutofixRequest} from "./types/app"
Expand Down Expand Up @@ -190,7 +190,7 @@ function formatFileContent(options, file, filePath) {
input: file.source,
env: hasElmFormatPathFlag
? process.env
: backwardsCompatibleElmFormatEnv()
: {...process.env, [pathKey]: backwardsCompatiblePath()}
}
);

Expand Down Expand Up @@ -237,61 +237,3 @@ ${result.stderr.toString()}`,
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)
};
}
3 changes: 2 additions & 1 deletion lib/elm-binary.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const chalk = require('chalk');
const which = require('which');
const spawn = require('cross-spawn');
const ErrorMessage = require('./error-message');
const {backwardsCompatiblePath} = require('./npx');

/**
* @import {Options} from "./types/options"
Expand All @@ -19,7 +20,7 @@ const ErrorMessage = require('./error-message');
async function getElmBinary(options) {
if (options.compiler === undefined) {
try {
return await which('elm');
return await which('elm', {path: backwardsCompatiblePath()});
} catch {
throw new ErrorMessage.CustomError(
// prettier-ignore
Expand Down
76 changes: 76 additions & 0 deletions lib/npx.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* A fast `npx` alternative.
*/

const path = require('node:path');
const getPathKey = require('path-key');

/**
* The key for `process.env` containing the path, generally `PATH`.
*
* @type {string}
*/
const pathKey = getPathKey();

/**
* Prepends `node_modules/.bin` up to the root directory to the PATH.
*
* @remarks
* 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`.
*
* However, `npx` adds all potential `node_modules/.bin` up to current directory to the
* beginning of PATH, for example:
*
* ```sh
* $ 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 unnecessary 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`/`npm exec`, npm scripts, or `--elm-format-path`.
* - 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 backwardsCompatiblePath() {
return [
...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);
}

module.exports = {
backwardsCompatiblePath,
pathKey
};
22 changes: 1 addition & 21 deletions test/jest-helpers/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,7 @@ async function internalExec(args, options = {}) {

try {
const result = await exec(
[
colorFlag,
cli,
reportMode(options),
colors(options),
...(args.includes('--compiler') ? [] : [elmPath()]),
args
].join(' '),
[colorFlag, cli, reportMode(options), colors(options), args].join(' '),
{
...options,
cwd: cwdFromOptions(options)
Expand All @@ -71,19 +64,6 @@ async function internalExec(args, options = {}) {
}
}

function elmPath() {
const elmPath = path.resolve(
__dirname,
'..',
'..',
'node_modules',
'.bin',
'elm'
);

return `--compiler=${elmPath}`;
}

/**
* @param {Options} options
*/
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.no-implicit-any.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,19 @@
"lib/sync-get.js",
"lib/sync-get-worker.js",
"lib/error-message.js",
"lib/path-helpers.js",
"lib/flags.js",
"lib/fs-wrapper.js",
"lib/hash.js",
"lib/help.js",
"lib/load-compiled-app.js",
"lib/min-version.js",
"lib/new-rule.js",
"lib/npx.js",
"lib/options.js",
"lib/os-helpers.js",
"lib/parse-elm.js",
"lib/parse-elm-worker.js",
"lib/path-helpers.js",
"lib/project-json-files.js",
"lib/promisify-port.js",
"lib/report.js",
Expand Down

0 comments on commit 3af193d

Please sign in to comment.