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

Don't require npx when running elm-format. Remove as peer-depdency. #458

Merged
merged 7 commits into from
Feb 5, 2021

Conversation

dillonkearns
Copy link
Owner

Fixes #442.

This change removes elm-format as a peer-dependency.

For reference, here are some other projects that have explored different approaches to this problem:

rtfeldman/node-test-runner#419

This is how elm-review currently runs elm-format and gives errors if it isn't found: https://github.com/jfmengels/node-elm-review/blob/6d0cf84ac554bf95bd21ecfac251e7a466ad2016/lib/autofix.js#L116-L170. It first tries to find it on the PATH, and then tries to run it with npx. But from conversations with @lydell, this is probably unnecessary, just checking on the PATH should be sufficient.

@dillonkearns
Copy link
Owner Author

This yields this error. Could be cleaner and more beautiful, but it does have the necessary information. And if for some reason the error wasn't just that it can't be found, it would provided the necessary context.

Image 2021-02-04 at 8 12 43 PM

elm-review does a search in the error string for "not found" and just shows a specific error message that it can't be found in that case. Is that robust enough and worth doing? I can't make up my mind on whether that's an improvement or a heuristic that could cover up information when there may be a problem. For example, what if elm-format can't find a binary it needs, like glibc, and prints out an error that includes "not found"?

I'm leaning towards leaving the error as is.

@lydell
Copy link
Contributor

lydell commented Feb 5, 2021

https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback

Never pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

For this reason, I never use .exec. Only .spawn. I played around with this on Windows the other day. Unfortunately, .spawn("elm-format") does not find elm-format – even though I run npx elm-graphql. In Windows, there’s a difference between spawning things directly and from a shell. More things are available through the shell. For example, if you install elm using the .exe installer, .spawn("elm") works. But if you install elm with npm, it’s not found (even if you use npx elm-graphql).

.spawn has a shell: true option that solves that issue. But it also brings us back to the original one:

If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

That’s why the cross-spawn package exists (which elm-review uses).

So – I’d recommend using cross-spawn. Both to fix the unsanitized input thing, and to support Windows.

Finally, if error.code === "ENOENT" the command could not be found (unless I’m completely mistaken – use it in elm-tooling-cli at least). So I guess you could use that to get a nicer error.

@dillonkearns
Copy link
Owner Author

@lydell you're a fountain of knowledge! Thank you so much for taking a look 🙏

@dillonkearns dillonkearns merged commit be107a0 into master Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't require NPX for elm-format
2 participants