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

Breaking change in icu_provider 1.2.0 #3332

Closed
Razican opened this issue Apr 15, 2023 · 5 comments · Fixed by #3333
Closed

Breaking change in icu_provider 1.2.0 #3332

Razican opened this issue Apr 15, 2023 · 5 comments · Fixed by #3333

Comments

@Razican
Copy link

Razican commented Apr 15, 2023

With the update from icu_provider 1.1.0 to 1.2.0, the pub use prelude::* has been removed:

https://docs.rs/icu_provider/1.1.0/src/icu_provider/lib.rs.html#199

This means that after a cargo update, we can no longer use the following syntax:

use icu_provider::{
    yoke::{trait_hack::YokeTraitHack, Yokeable},
    zerofrom::ZeroFrom,
};

This is a semver-incompatible change, unfortunately :(

@sffc
Copy link
Member

sffc commented Apr 15, 2023

Was noted in a review in #3324 (comment); the API in question was #[doc(hidden)] which we generally ignore for semver purposes, but if you broke on upgrading we should probably add it back, still as #[doc(hidden)]. I'm fine making a 1.2.1 release of that crate to add back those two APIs.

@Razican
Copy link
Author

Razican commented Apr 15, 2023

This is interesting. Maybe we should just be importing the crates directly? Since #[doc(hidden)] stuff is not considered for backwards compatibility, this could happen again in the future.

Also, even when using the prelude::* imports, we got some compile errors, which we have to re-check.

@Razican
Copy link
Author

Razican commented Apr 15, 2023

You can check here the build errors we're getting after importing yoke and zerofrom from the prelude sub-module: https://github.com/boa-dev/boa/actions/runs/4709275331/jobs/8352236237?pr=2826

@sffc
Copy link
Member

sffc commented Apr 15, 2023

Oof, it looks like we lost a + ?Sized in the LocaleCanonicalizer constructor

error[E0277]: the size for values of type `dyn BufferProvider` cannot be known at compilation time
   --> boa_engine/src/context/icu.rs:62:67
    |
62  |                 LocaleCanonicalizer::try_new_with_buffer_provider(&**buffer)
    |                 ------------------------------------------------- ^^^^^^^^^ doesn't have a size known at compile-time
    |                 |
    |                 required by a bound introduced by this call
    |
    = help: the trait `Sized` is not implemented for `dyn BufferProvider`
note: required by a bound in `LocaleCanonicalizer::try_new_with_buffer_provider`
   --> /Users/runner/.cargo/registry/src/github.jparrowsec.cn-1ecc6299db9ec823/icu_locid_transform-1.2.0/src/canonicalizer.rs:240:20

Since you guys have a lot of call sites, we should add "test with Boa" to our release checklist...

@Razican
Copy link
Author

Razican commented Apr 16, 2023

Regarding the exports of yoke and zerofrom, we have decided that we will directly import the dependencies in boa, so there is no need to revert that part.

bors bot pushed a commit to boa-dev/boa that referenced this issue Apr 18, 2023
This PR upgrades ICU to 1.2.

Unfortunately we still have some breaking changes, so this is being handled in unicode-org/icu4x#3332


Co-authored-by: jedel1043 <[email protected]>
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 a pull request may close this issue.

2 participants