Skip to content

history: Use thread_local!'s lazy initialization instead of RefCell<Option<_>> #358

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

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jul 23, 2023

Closes #356.

@ranile
Copy link
Collaborator

ranile commented Jul 24, 2023

I'm fine with the change but MSRV bump makes me not want to make this change. It means that not only gloo but also libraries that depend on gloo have to bump their MSRV.

Can we use the once_cell crate if OnceCell is not available from std?

@jplatte
Copy link
Contributor Author

jplatte commented Jul 24, 2023

Sure, but how should Cargo decide whether to pull in the dependency or not? If I make it an optional dependency, this is still an MSRV bump change for people who were using default-features = false. Is that acceptable? We could also pull in once_cell unconditionally, it's a crate that very oftens ends up in people's dep tree anyways.

@futursolo
Copy link
Collaborator

How about we use once_cell::unsync::Lazy for all versions until it becomes available in stable? This avoids a MSRV bump and promotes lazy initialisation, which fits this use case better.

I do not feel comfortable that we should bump MSRV to the latest stable given latest stable has issues with WASM-bindgen (rust-lang/rust#111888).

@jplatte
Copy link
Contributor Author

jplatte commented Jul 24, 2023

I looked at the docs for thread_local! more closely and found that it supports lazy initialization all on its own. Have updated the PR with an implementation that's simpler than everything we have discussed before :)

@jplatte jplatte changed the title history: Use OnceCell instead of RefCell<Option<_>> for thread locals history: Use thread_local!'s lazy initialization instead of RefCell<Option<_>> Jul 24, 2023
Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Thanks!

@ranile ranile merged commit 345ac09 into rustwasm:master Jul 24, 2023
@ranile
Copy link
Collaborator

ranile commented Jul 24, 2023

Released in gloo-history 0.1.5

@jplatte jplatte deleted the once-cell branch July 24, 2023 17:35
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.

Use OnceCell for gloo-history
3 participants