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

Restrict the dependency on indexmap to v2 only #51

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

its-the-shrimp
Copy link
Contributor

Allowing both indexmap v1 & v2 causes a lot of annoyance when the version of indexmap used by this crate and the one used by other crates in the dependency tree don't match.
See also yewstack/yew#3659

.github/workflows/rust.yml Outdated Show resolved Hide resolved
Copy link
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

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

lgtm

Are you 100% sure this will fix the issue yewstack/yew#3659 ? If you have managed to reproduce and have tested all the scenarios and you are sure this will fix the issue this is great. But it's a breaking change which requires bumping the major, you will also need to make a Yew release.

@ranile
Copy link
Member

ranile commented Jun 4, 2024

I highly doubt it fixes yewstack/yew#3659. The error comes because cargo doesn't update dependencies of a dependency when that dependency is updated manually via editing Cargo.toml instead of using cargo update

@its-the-shrimp
Copy link
Contributor Author

however, the actual error is that Rust considers indexmap v1.9 & indexmap v2.0 different crates, if implicit-clone didn't allow v1.*, this wouldn't have been a problem

@cecton
Copy link
Member

cecton commented Jun 5, 2024

so.... shall we merge or we need more feedbacks from other maintainers?

With all the sh*t I saw with cargo when allowing multiple versions like that, I believe this is still broken. It's more of a cargo issue imo... but in the meantime I don't think it hurts to force the version of indexmap to 2.x.y. The only downside is that it will solve issues for the next versions of Yew only. People using 0.21 will need to fix their dependency tree manually with a command that looks like this one:

[0] [09:34:06] ~/.c/c/tmp-wlG0hL HEAD > cargo update -p indexmap --precise 2.2.6
error: There are multiple `indexmap` packages in your project, and the specification `indexmap` is ambiguous.
Please re-run this command with one of the following specifications:
  [email protected]
  [email protected]
[101] [09:34:11] ~/.c/c/tmp-wlG0hL HEAD > cargo update -p [email protected] --precise 2.2.6
    Updating crates.io index
    Removing autocfg v1.3.0
    Removing hashbrown v0.12.3
    Removing indexmap v1.9.3

(just tested on a temporary project using cargo temp implicit-clone+map, then downgrading indexmap, then adding indexmap 2 as dependency)

@Figo57
Copy link

Figo57 commented Jun 12, 2024

well.. that error stayed with me for 2weeks. when It was gone... I...

@cecton
Copy link
Member

cecton commented Jun 20, 2024

well.. that error stayed with me for 2weeks. when It was gone... I...

it works why

@its-the-shrimp
Copy link
Contributor Author

its-the-shrimp commented Jun 20, 2024

I honestly have no clue how to reproduce the issue that's being solved with this PR, until we find a stable way to do so, it'll have to wait, and we'll have to keep looking...

@Figo57
Copy link

Figo57 commented Jun 26, 2024

好吧..这个错误伴随了我2个星期。当它消失时...我...

它为什么有效

There was no other way At least.. but I feel happy than compiling error GG😄

@tsal
Copy link

tsal commented Jul 12, 2024

honestly have no clue how to reproduce the issue that's being solved with this PR

I was able to reproduce it by performing the steps in the yew quick start using Rust 1.79 out of the box. I could not get past the first build of the App code.

@cecton
Copy link
Member

cecton commented Jul 12, 2024

@its-the-shrimp I don't think I want to bother testing more and it's not really useful to keep the retro-compatibility here so I'd suggest we merge and see with next Yew version. (Not the current version as it might break things for others). What do you think?

@its-the-shrimp
Copy link
Contributor Author

I've just tried the Yew quick-start defined here: https://yew.rs/docs/next/getting-started/build-a-sample-app#setting-up-the-application-manually, and I couldn't get any error, but the error is definitely caused by allowing v1 alongside v2, so I think we should merge this

@cecton cecton merged commit 14f4da6 into yewstack:main Jul 12, 2024
6 checks passed
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.

5 participants