Install elm-json using elm-tooling #480
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also install the dev dependencies elm and elm-format using elm-tooling.
|
I might not get to my laptop to look at this before Christmas so bare with me. I have already gone from skeptical to keen though so nice work 👍 |
|
No worries! No rush to do anything before Christmas. Or this year. We’ll get back to it in 2021! 🎅 |
harrysarson
approved these changes
Jan 2, 2021
harrysarson
added
status: waiting for author
harrysarson
added
status: waiting for review
harrysarson
added
status: waiting for author
harrysarson
pushed a commit
that referenced
this pull request
Jan 22, 2021
Before:
```
❯ ../bin/elm-test
-- UNFINISHED DEFINITION - /Users/lydell/forks/node-test-runner/example-application/tests/TestsPassing.elm
I got stuck while parsing the `x` definition:
3| x
^
I was expecting to see an argument or an equals sign next.
Here is a valid definition (with a type annotation) for reference:
greet : String -> String
greet name =
"Hello " ++ name ++ "!"
The top line (called a "type annotation") is optional. You can leave it off if
you want. As you get more comfortable with Elm and as your project grows, it
becomes more and more valuable to add them though! They work great as
compiler-verified documentation, and they often improve error messages!
Compiling >
`elm make` failed with exit code 1.
```
After:
```
❯ ../bin/elm-test
-- UNFINISHED DEFINITION - /Users/lydell/forks/node-test-runner/example-application/tests/TestsPassing.elm
I got stuck while parsing the `x` definition:
3| x
^
I was expecting to see an argument or an equals sign next.
Here is a valid definition (with a type annotation) for reference:
greet : String -> String
greet name =
"Hello " ++ name ++ "!"
The top line (called a "type annotation") is optional. You can leave it off if
you want. As you get more comfortable with Elm and as your project grows, it
becomes more and more valuable to add them though! They work great as
compiler-verified documentation, and they often improve error messages!
`elm make` failed with exit code 1.
```
Notice the `Compile >` line near the bottom in the “Before” output. That’s not supposed to be there.
I fixed this by reverting the change here: #480 (comment)
This is pretty tricky. I’ll try to explain.
So we print this `Thing > Other thing > Last thing` progress line, where each segment appears over time. For this reason, the terminal cursor never moves to the next line, so we can overwrite the line over and over. Once done, we move to the next line so the test output can print below the (finished) progress line.
But what happens if there’s an error? We don’t want to end up with something like:
```
Thing > Other thingError: Can’t do this and that for:
more lines of error
Maybe try this and that?
```
So we want to move to the next line when printing an error as well. `progressLogger.logLine('')` worked for this case most of the time: It would reprint `Thing > Other` (which technically was unnecessary but couldn’t be seen) and then moved to the new line. Another thing I noticed while writing this: The empty string would actually result in `This > Other > ` (an unwanted trailing `>` separator).
But the thing is that we can’t actually know that the cursor is still on the cursor line. Why? Because `elm make` might have printed to stderr (in case of compilation errors in the user’s code). We don’t know if it has either – we can’t capture its stderr because then we would lose colors.
So a solution that works in both cases is to _not_ reprint the progress line and instead only move to the next line. (That results in an extra blank line for the compiler errors but I don’t think it matters.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR uses elm-tooling to install elm-json instead of the elm-json npm package.
Look at this size improvement:
Size was measured by running
du -sh node_modulesafter installing node-test-runner (and nothing else) on MacOS.The elm-json binary itself around 2 MB on MacOS.
The huge space drop comes from not depending on
binwrapanymore. It has some pretty have dependencies.Benefits of using elm-tooling:
npm install. And if the user uses elm-tooling to install elm and elm-format as well, all three tools are downloaded in parallel. Similar PR for elm-review.The only noticeable difference is when the elm-json executable is transferred from the Internet to the user’s machine.
Previously, it happened during
npm install. Unless you use ignore-scripts – then it happened the first timeelm-jsonwas executed. Now, it happens right beforeelm-jsonis about to be executed. Unless the user already downloaded elm-json to~/.elm/elm-tooling/before.Given that node-test-runner executes both elm and elm-json, which both can make network requests, I think it’s fine to also execute elm-tooling’s
getExectuablefunction which also can make network requests. node-test-runner continues to work offline once all needed things exist on the user’s machine.binwrap uses the request module, while elm-tooling uses curl if available, otherwise wget and finally Node.js’ https module. If the user used request’s environment variables for proxies, that should probably just work since curl seems to use the same environment variables. But there might be some little difference. Either way, both curl and wget also support config files that lets the user alter how they work as needed, which is a good thing.
Previously, the elm-json binary was downloaded to
./node_modules/elm-json/unpacked-bin/. Now it’s downloaded to~/.elm/elm-tooling/elm-json/0.2.8/. Elm and elm-json write to~/.elmtoo, so it should be fine. (elm-tooling supportsELM_HOMEand Windows.)The plan now is to:
elm-toolingto the @elm-tooling org so I don’t become a single point of failure.elm-toolingin Slack and Discourse. So far I haven’t talked much about it, waiting for these two PRs to be made.