zwilias / elm-json

Install, upgrade and uninstall Elm dependencies
MIT License
183 stars 9 forks source link

Installing devDependencies does not remove them from indirect dependencies #22

Closed jfmengels closed 4 years ago

jfmengels commented 4 years ago

Summary

When you install things using elm-json --test you do not get the same results as elm-test install or elm-test init when the dependency to be installed is already in the indirect dependencies.

Explanation

In elm-review, when you run elm-review init we generate the following empty JSON

{
    "type": "application",
    "source-directories": [
        "src"
    ],
    "elm-version": "0.19.1",
    "dependencies": {
        "direct": {
            "elm/core": "1.0.2",
            "elm/json": "1.1.3"
        },
        "indirect": {}
    },
    "test-dependencies": {
        "direct": {},
        "indirect": {}
    }
}

then run

elm-json install --yes jfmengels/elm-review@2 stil4m/elm-syntax@7 elm/project-metadata-utils@1 -- <pathToElmJson>

(thanks again for implementing the @major version a few months ago! :heart: )

and that gives us

{
    "type": "application",
    "source-directories": [
        "src"
    ],
    "elm-version": "0.19.1",
    "dependencies": {
        "direct": {
            "elm/core": "1.0.2",
            "elm/json": "1.1.3",
            "elm/project-metadata-utils": "1.0.1",
            "jfmengels/elm-review": "2.0.0",
            "stil4m/elm-syntax": "7.1.1"
        },
        "indirect": {
            "elm/html": "1.0.0",
            "elm/parser": "1.1.0",
            "elm/random": "1.0.0",
            "elm/time": "1.0.0",
            "elm/url": "1.0.0",
            "elm/virtual-dom": "1.0.2",
            "elm-community/json-extra": "4.2.0",
            "elm-community/list-extra": "8.2.3",
            "elm-explorations/test": "1.2.2",
            "rtfeldman/elm-hex": "1.0.0",
            "rtfeldman/elm-iso8601-date-strings": "1.1.3",
            "stil4m/structured-writer": "1.0.2"
        }
    },
    "test-dependencies": {
        "direct": {},
        "indirect": {}
    }
}

Notice :arrow_up: that "elm-explorations/test": "1.2.2" has been added to indirect dependencies. That is because jfmengels/elm-review provides test helpers and as elm-explorations/test as a dependency.

Because I wanted to make it as easy as possible to get started writing tests for elm-review rules (things that you make with the library), I pre-add elm-explorations/test using

elm-json install --test --yes elm-explorations/test@1 -- <pathToElmJson>

and that gives the following elm.json

{
    "type": "application",
    "source-directories": [
        "src"
    ],
    "elm-version": "0.19.1",
    "dependencies": {
        "direct": {
            "elm/core": "1.0.5",
            "elm/json": "1.1.3",
            "elm/project-metadata-utils": "1.0.1",
            "jfmengels/elm-review": "2.0.0",
            "stil4m/elm-syntax": "7.1.1"
        },
        "indirect": {
            "elm/html": "1.0.0",
            "elm/parser": "1.1.0",
            "elm/random": "1.0.0",
            "elm/time": "1.0.0",
            "elm/url": "1.0.0",
            "elm/virtual-dom": "1.0.2",
            "elm-community/json-extra": "4.2.0",
            "elm-community/list-extra": "8.2.3",
            "elm-explorations/test": "1.2.2",
            "rtfeldman/elm-hex": "1.0.0",
            "rtfeldman/elm-iso8601-date-strings": "1.1.3",
            "stil4m/structured-writer": "1.0.2"
        }
    },
    "test-dependencies": {
        "direct": {
            "elm-explorations/test": "1.2.2"
        },
        "indirect": {}
    }
}

So everything looks good, except that elm-explorations/test is still in the indirect dependencies, and also in the direct dependencies, which elm-test doesn't like when it is running.

-- ERROR IN DEPENDENCIES ---------------------------------------------- elm.json

It looks like the dependencies elm.json in were edited by hand (or by a 3rd
party tool) leaving them in an invalid state.

Try to change them back to what they were before! It is much more reliable to
add dependencies with elm install or the dependency management tool in
elm reactor.

Please ask for help on the community forums if you try those paths and are still
having problems!

When you run elm-test init, elm-test removes elm-explorations/test from the indirect dependencies and adds it to the direct devDependencies. Then, running tests work.

You get the following elm.json:

{
    "type": "application",
    "source-directories": [
        "src"
    ],
    "elm-version": "0.19.1",
    "dependencies": {
        "direct": {
            "elm/core": "1.0.2",
            "elm/json": "1.1.3",
            "elm/project-metadata-utils": "1.0.1",
            "jfmengels/elm-review": "2.0.0",
            "stil4m/elm-syntax": "7.1.1"
        },
        "indirect": {
            "elm/html": "1.0.0",
            "elm/parser": "1.1.0",
            "elm/random": "1.0.0",
            "elm/time": "1.0.0",
            "elm/url": "1.0.0",
            "elm/virtual-dom": "1.0.2",
            "elm-community/json-extra": "4.2.0",
            "elm-community/list-extra": "8.2.3",
            "rtfeldman/elm-hex": "1.0.0",
            "rtfeldman/elm-iso8601-date-strings": "1.1.3",
            "stil4m/structured-writer": "1.0.2"
        }
    },
    "test-dependencies": {
        "direct": {
            "elm-explorations/test": "1.2.2"
        },
        "indirect": {}
    }
}
zwilias commented 4 years ago

I consider the elm-test behaviour to be wrong here. For reference, elm install does the same thing here - it ignores whatever is in test-dependencies because the information there should not be relevant to building your app.

In fact, the behaviour you describe was the one that elm-json exhibited at first, but I changed it to match the more logically consistent behaviour of elm install, even if that means giving up some compatibility with elm-test. Some discussion here: https://github.com/elm/compiler/issues/1860#issue-384836248

Long story short: dependencies should list everything needed to build the application, which in this case includes elm-explorations/test.

jfmengels commented 4 years ago

That sounds right :+1:

I'll delete the indirect dependency manually for now and open an issue in rtfeldman/node-test-runner when I have time.

Thank you for the insight :)

zwilias commented 4 years ago

This was a really well written, clearly illustrated issue, so thank you for that!