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

Use of custom scalar codecs requires that elm executable be available in PATH #276

Closed
rjdellecese opened this issue Jan 10, 2020 · 9 comments

Comments

@rjdellecese
Copy link

The custom scalar codecs feature is great! But I've discovered that when using it, it adds a new requirement to my build process through its use of elm make when verifying the scalar codecs file, here:

childProcess.execSync(`elm make ${verifyDecodersFile} --output=/dev/null`, {

I would prefer to be able to use the same Elm executable that is used to compile my Elm project (which for me and I would guess most people is the one in node_modules/.bin/elm). I can accomplish this by adding that to the PATH of my build environment, but I wonder if there is a better way, like passing this path as a command-line argument to the elm-graphql executable? Or perhaps the executable should also check for an elm binary in that location?

@dillonkearns
Copy link
Owner

Hello @rjdellecese, thank you for opening the issue. Thanks an excellent point! Would you be up for making a PR to try to refine this? I'd love some help to improve things here.

Some research on the different Elm bin lookup strategies

I'm not sure what the most reliable way would be, those are good ideas, though.

I dug into it a bit because I remembered seeing something like the node_modules/.bin lookup in some core team member's CLI tools. But it looks like Richard had been using that exact technique (first lookup node_modules/.bin/elm, then fallback to PATH) with node-elm-compiler. But he reverted that in this commit. I couldn't find an explanation of why:

rtfeldman/node-elm-compiler@bdeb661

Note that this is the project that elm-test depends on for running Elm (which it would probably make sense to use in this project as well). The only changes that elm-test made in order to support that in the CLI was to add a flag that lets you configure the elm binary:

rtfeldman/node-test-runner@ece680b

(Here are the commits around there, I couldn't find anything else related: https://github.com/rtfeldman/node-test-runner/commits/4464502226aabd13e74d86c3b43e81c8107fe22c#diff-b9cfc7f2cdf78a7f4b91a753d10865a2). Note that node-test-runner is the project for the elm-test CLI code.

You can also see where it uses it in the latest version of the code here. It looks like it just checks the PATH unless you provide a path to the compiler in the CLI flags:

https://github.com/rtfeldman/node-test-runner/blob/master/lib/elm-test.js#L100

Next steps

I'm thinking that just adding an option for --compiler flag (as elm-test does) is the best option. I realized that what I do most of the time is run elm-graphql and elm-test from my NPM scripts, so it sets up the PATH with the right binaries from node_modules/.bin. So I think that the --compiler option should be sufficient to handle any other use cases.

Let me know if you're interested in giving it a shot!

I think this is the code you need to add it:

@rjdellecese
Copy link
Author

Wow, this is great! Thank you for doing all that research @dillonkearns! I’d be happy to take a crack at it—I’ll aim to give it a shot in the next few days.

@lydell
Copy link
Contributor

lydell commented Feb 4, 2021

So the use case here seems to be “I want elm-graphql to use my locally installed elm”? If so, the solution is easy! Execute elm-graphql using npx (npx elm-graphql foo bar)!

npx adds the local node_modules/.bin/ to $PATH for the command it executes, so elm-graphql will automatically pick up your local elm version, without neither elm-graphql nor you having to do anything special!

@rjdellecese Could you try running using npx (or yarn or npm run or what have you) and report how it works?

@rjdellecese
Copy link
Author

@lydell I no longer have access to the project whence this issue arose, so I can't test it there, unfortunately. But I do know that it was using yarn, not npm, and I didn't think that yarn had an equivalent for npx (yarnpkg/yarn#3937)?

Pinging @shamshirz and @hritchie, in case they are able/have the time to test this out on the codebase where I originally ran into this issue!

@lydell
Copy link
Contributor

lydell commented Feb 4, 2021

@rjdellecese In this case, the equivalent of npx elm-graphql in yarn is yarn elm-graphql. Yarn works roughly like this:

  1. Is the arg a built-in command (such as install)? Then run that.
  2. Does the arg match a script in "scripts" in package.json? Then run that.
  3. Does the arg match a binary in node_modules/.bin/? Then run that.
  4. Error.

@rjdellecese
Copy link
Author

@lydell per yarnpkg/yarn#733, it does seem like yarn my-thing or yarn run my-thing should add node_modules/.bin/ to the PATH—at least if my-thing is a script. I wonder if it does not function that way if my-thing is an executable in node_modules/.bin to begin with? I believe we were using yarn run elm-graphql in CI when I ran into this issue, so either I'm mistaken somehow (certainly possible) or there is some lurking variable here! Perhaps if we had wrapped the execution of elm-graphql in a script, it would have worked as desired?

This seems verifiable in a pretty straightforward way, but I'm not sure that I'll have the time to anytime soon. Given that, I'm happy to have this issue closed until/unless someone can investigate further!

@lydell
Copy link
Contributor

lydell commented Feb 4, 2021

I wonder if it does not function that way if my-thing is an executable in node_modules/.bin to begin with?

I works even in that case.

❯ printf '#!/bin/bash\necho "$PATH"' > node_modules/.bin/tst

❯ chmod +x node_modules/.bin/tst

❯ yarn tst
yarn run v1.22.10
$ /Users/lydell/project/app/node_modules/.bin/tst
/var/folders/rb/wgmns9nj7xx58nj4fxlbkx5r0000gn/T/yarn--1612460150570-0.9759789728445636:/Users/lydell/project/app/node_modules/.bin:<SNIP>
✨  Done in 0.40s.

@rjdellecese
Copy link
Author

Well then, I'm not sure what the issue was, and without access to the problem code anymore, I can't explore this any further! So I'll close this issue.

Thanks for your input @lydell—I'm sure it will be helpful should anyone else run into a problem like this one.

@kelvinma
Copy link

@rjdellecese To close the loop on this, going from node_modules/.bin/elm-graphql ... to yarn elm-graphql ... did the trick!

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 a pull request may close this issue.

4 participants