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

Widen constraint for elm-solve-deps-wasm #636

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lishaduck
Copy link

@lishaduck lishaduck commented Jul 9, 2024

Allow elm-solve-deps-wasm v2.

CC: @lydell.

Otherwise, npm whines.
I'm rather surprised this didn't complain before.

To be clear: this kept the same versions locked, it just updated the metadata.
@lydell
Copy link
Collaborator

lydell commented Jul 9, 2024

Is this to save space in node_modules/ for those who install both elm-review and elm-test?

@lishaduck
Copy link
Author

lishaduck commented Jul 9, 2024

Is this to save space in node_modules/ for those who install both elm-review and elm-test?

Yeah. If we allow both versions, then it'll install the latest.
I did the same in elm-review. This way, it's not breaking, we don't force anyone to upgrade. However, npm will default to v2.

If you install elm-test, then elm-review, currently you'll only get one, outdated elm-solve-deps-wasm, but if you install elm-review, then elm-test, currently you'll get two, one outdated and one new. This fixes that so that npm will always default to the new one.

lishaduck added a commit to lishaduck/node-elm-review that referenced this pull request Sep 25, 2024
Manually edited the `package-lock.json` to force `elm-solve-deps-wasm` v2.
Then, regenerated the lockfile to give `elm-test` the old version.
Workaround for rtfeldman/node-test-runner#636.
@lishaduck
Copy link
Author

@lydell, ping :)

@lydell
Copy link
Collaborator

lydell commented Sep 25, 2024

I’ve been waiting for a release of elm-review with bumps :)

@lishaduck
Copy link
Author

I think we're currently targeting a major (dropping Node 10&12), so it'll probably be a while before we release, but this breaks our CI (we depend on 2.0.0's types internally).

Alternatively, @jfmengels, would you be ok with backporting the elm-solve-deps-wasm upgrade to elm-review 2.x?
I think that'd just need a new branch off jfmengels/node-elm-review@5f2fcc3, cherrypick jfmengels/node-elm-review@79e6b9a, and get tests to pass (i.e., bump elm-syntax in tests).

@jfmengels
Copy link
Contributor

@lishaduck I think we're still on course for v2 for elm-review. We're officially dropping support for v10/12, but they were already broken for a while (so the only difference would be the "officially" part). I think I'd be happy to release a new version. What do you think @lishaduck?

@lydell
Copy link
Collaborator

lydell commented Sep 25, 2024

There’s no rush with this, right? The new version does not bring any improvements to end users?

@lishaduck
Copy link
Author

lishaduck commented Sep 25, 2024

There’s no rush with this, right? The new version does not bring any improvements to end users?

elm-solve-deps-wasm v2 improves the bindings by fixing incorrect types, but doesn't change the behavior. The incorrect types breaks elm-review's CI, but as there's not any improvements to elm-solve-deps-wasm for end users, there isn't a new elm-review bump planned. Because we depend on elm-test, though, npm prefers installing the old version. It's not terribly important (I can manually fix the lockfile, and npm'll go with it, as I have been), but it'd be nice to fix it.

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 this pull request may close these issues.

3 participants