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

Leverage elm-json solve #356

Closed
wants to merge 4 commits into from
Closed

Leverage elm-json solve #356

wants to merge 4 commits into from

Conversation

zwilias
Copy link
Collaborator

@zwilias zwilias commented Apr 27, 2019

Fixes #328, fixes #277, fixes #273, fixes #323, fixes #374

I recently released elm-json, a tool for interacting with elm.json files. One of the available subcommands is solve: Given an elm.json file (for a package or application), it tries to generate a single set of direct and indirect dependencies that satisfy all constraints. It has 2 flags that make it useful for node-test-runner:

  • --test => Includes the test-dependencies and promotes them to direct dependencies
  • --extra => Allows providing an extra set of packages that need to be direct dependencies of the final result

Since elm-json can be installed through npm and has binaries available for OS X, Windows and Linux (statically linked), it seems like a fine candidate to use in node-test-runner for calculating the final set of dependencies needed for the generated application.


I've tested this on all the failure cases listed in the linked tickets, as well as a number of my own packages and applications. As far as I can tell, it just works.

With this in place, we could also start thinking about running tests for packages for both the minimal allowed versions (by passing the --minimize flag to elm-json solve) and the maximal allowed versions. I did not include such exploration in this PR, but want to note that this now becomes feasible.


Cons of this approach:

  • extra (binary) dependency (but we already have elmi-to-json, so that's not a blocking factor)
  • very young package (though I suspect the elm-json solve interface will not change in the foreseeable future)
  • depending on a tool written in Rust increases the barrier for dealing with issues related to interactions with elm-json

@avh4
Copy link
Collaborator

avh4 commented Apr 27, 2019

Cons of this approach

I think another notable one is that elm-json is written in rust -- I think haskell and javascript are are already reasonably involved in the elm ecosystem, but having a very different language like rust required for a central tool like elm-test add notable risk. (Specifically, if there are issues in elm-test due interactions with elm-json, there is a very large barrier of entry for someone to be able to debug both projects.)

With this in place, we could also start thinking about running tests for packages for both the minimal allowed versions and the maximal allowed versions

That sounds cool!

@avh4
Copy link
Collaborator

avh4 commented Apr 27, 2019

(Also, I'm not sure why the Travis-CI build isn't running for this PR)

@zwilias
Copy link
Collaborator Author

zwilias commented Apr 27, 2019

having a very different language like rust required for a central tool like elm-test add notable risk

This is true! To be fair, there are experiments with actually rewriting the elm-test CLI in Rust, so I feel fairly confident that a reasonable amount of people who could potentially debug issues should they arise, are involved with the node-test-runner project. On the other hand, that experiment might have been abandoned for exactly this reason 😅

(edit: Added it to the cons list)

(Also, I'm not sure why the Travis-CI build isn't running for this PR)

I have no idea why, but travis was disabled for PR's. I've re-enabled it after I noticed it wasn't running. I still have a commit pending for removing the version.js file, so 🤞 that the tests will trigger when I push that!

(edit: They're running now! 🎉 )

Copy link
Collaborator

@harrysarson harrysarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

The existing logic for inferring the correct dependences is dubious to say the least and has been causing issues for folk using this package. "solving" the dependency tree is probably something that the elm compiler (or package manager if that becomes a separate thing) will do in the long term future and, as such, depending on an external program is something that a test runner will have to do eventually.

I think this changeset is a big improvement.

@harrysarson
Copy link
Collaborator

makes #352 redundant

I do believe that #352 is orthogonal to this and could both be merged. #352 updates the packages that the test runner uses and adds some tests.

@zwilias
Copy link
Collaborator Author

zwilias commented May 7, 2019

@harrysarson The tests make sense, it's the changes to that elm.json file that are made redundant. I believe that file isn't used any longer as of this PR. In that sense, this PR fixes the same issue - it ensures a compatible version of elm/core is installed, even when missing in the package under test.

Perhaps I should remove that elm.json file from the repo in this PR as well. I'll have a look at doing so 👍

@harrysarson
Copy link
Collaborator

Perhaps I should remove that elm.json file from the repo in this PR as well.

If that is possible it would be a very good idea to do so. Keeping this file up to date is a pain.

@zwilias
Copy link
Collaborator Author

zwilias commented May 8, 2019

If that is possible it would be a very good idea to do so.

I'm less convinced. As a means of "propagating constraints" - yeah, that's painful and shouldn't be happening. But as an elm.json that's useful when actually working on the Elm code, it seems useful to have. It shouldn't be harder to keep up to date than any other elm.json.

@rtfeldman thoughts?

@harrysarson
Copy link
Collaborator

To unblock this, I propose:

  • elm-test detects if elm-json is installed.
  • If so, it uses elm-json as per this PR.
  • Otherwise, it uses the existing logic (the logic that this pull request deletes).
  • If the existing logic fails, we append a little hint to the error message suggesting that the user can install elm-json.

Thoughts?

@harrysarson
Copy link
Collaborator

Bump :)

1 similar comment
@harrysarson
Copy link
Collaborator

Bump :)

@mpizenberg
Copy link
Contributor

mpizenberg commented Mar 30, 2020

One important cons is that elm-json solve needs network access and takes a lot of time (I guess because I have poor connectivity here). This would prevent usage of elm-test in an offline context. Is it possible to have an offline version that tries to solve constraints with already installed packages?

@harrysarson
Copy link
Collaborator

That is a good point.

Saying that, elm-test runs the elm compiler under the hood so running it offline is not guaranteed to work (i.e. the elm compiler may decide it needs to download packages which requires internet access).

@harrysarson
Copy link
Collaborator

@zwilias could you chime in on this:

One important cons is that elm-json solve needs network access and takes a lot of time (I guess because I have poor connectivity here). This would prevent usage of elm-test in an offline context. Is it possible to have an offline version that tries to solve constraints with already installed packages?

I think you recently released a new version of elm-json that solves this issue.


Regarding this pull request, I now have commit rights to this repository! The cons:

Cons of this approach:

  • extra (binary) dependency (but we already have elmi-to-json, so that's not a blocking factor)
  • very young package (though I suspect the elm-json solve interface will not change in the foreseeable future)
  • depending on a tool written in Rust increases the barrier for dealing with issues related to interactions with elm-json

are, I think, worth accepting for the improvements this PR brings. Especially because elm-json is no longer so young and (subjectively) I think rust knowledge has increases across the elm community.

Therefore, I am inclined to merge this (after a rebase) in two to four weeks time unless anyone raises an further objections before then!

@zwilias
Copy link
Collaborator Author

zwilias commented Sep 25, 2020

There is indeed an offline mode, however, elm-json also just does a whole lot less network io now (by caching elm.json files where possible). Even on bad connections, using elm-json should be very much feasible.

It might potentially be interesting to look into caching the generated elm.json and only regenerating if the input elm.json changed, but that feels like an optimization in elm-test land, rather than in elm-json :)

@harrysarson
Copy link
Collaborator

Thanks @zwilias, that sounds great 👍 as far as I can tell there are no blockers to merging soon then.

@lydell
Copy link
Collaborator

lydell commented Oct 14, 2020

Since #451 was merged, we can close this one.

@zwilias
Copy link
Collaborator Author

zwilias commented Oct 14, 2020

#451 did this, but better. Thanks @lydell!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants