search

avh4 / elm-upgrade

Upgrade Elm projects
https://www.npmjs.com/package/elm-upgrade
MIT License
122 stars 15 forks source link

make upgrading work for elm 0.19.1 #83

Closed ChristophP closed 4 years ago

ChristophP commented 4 years ago

fixes #82

open question: The elm compiler in version 0.19.1 seems to emit an error when elm/json is not a direct dependency when an application uses ports or flags. To make it work I moved elm/json to the direct dependencies.

@avh4 Am I understanding this issue right or could there be cases when having elm/json as a direct dependency is not desirable?

ChristophP commented 4 years ago

The more I look through the elm compiler code the more it seems to me that elm/json is required as a dependency when the elm.json type is application. I just don't know why I am getting the Missing Dependency when when elm/json is an indirect dependency, since that should be ok as well. https://github.com/elm/compiler/blob/3c618045431de75d6029cb65997113247e4a23f7/builder/src/Elm/Outline.hs#L207

ChristophP commented 4 years ago

Ok I moved elm/json back to indirect-dependencies. Everything should work now for 0.19.0 and 0.19.1. It turns out the reason why it wasn't working before when elm/json was an indirect dependency was that my source directories looked like this:

"source-directories": [
        ".",
        "./src"
    ],

This caused elm-upgrade not to find the source directories when matching certain rules here. The new elm 0.19.1 compiler actually warns about source directories that contain other source directories. So I guess we can ignore this case. But if you think it's important I'll add another issue for it.

ChristophP commented 4 years ago

@avh4 The PR is ready for review, whenever you have time.

avh4 commented 4 years ago

Thanks! I also updated the tests for Elm 0.19.1 here: https://github.com/avh4/elm-upgrade/pull/84/commits/f9a2c285a0c03ad4a8221f4c67356086e6efec75

ChristophP commented 4 years ago

Ah tests weren't failing for me otherwise I would've taken care of that. Great, I saw you also added code to update straight to the version of the detected binary. Nice :+1:

avh4 commented 4 years ago

The tests were passing originally. I just updated the tests to use Elm 0.19.1 instead of using Elm 0.19.0 so that the tests covered the new behavior.

And yeah, your code made it work for upgrading from Elm 0.18 code. I added some stuff to make it also work when upgrading the dependencies of a project that was already on Elm 0.19.

ChristophP commented 4 years ago

Ah, so you can also upgrade 0.19.0 -> 0.19.1 . Great :-)