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

Add wasm support #127

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Add wasm support #127

merged 2 commits into from
Sep 7, 2023

Conversation

Stygmates
Copy link
Contributor

@Stygmates Stygmates commented Sep 6, 2023

Chrono's wasm support wasn't enabled, probably because the docs were outdated and it is currently not listed in the crate's default features but is present in the README .

chronotope/chrono#1260 updates the docs on their side. I can also make a PR for oauth2-rs if you want.

@ramosbugs
Copy link
Owner

Interesting, what was the impact of not having this feature enabled? I would have assumed the WASM CI build step would have failed

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.42% ⚠️

Comparison is base (414eb35) 52.01% compared to head (a2b9015) 51.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   52.01%   51.60%   -0.42%     
==========================================
  Files          16       16              
  Lines        2059     2056       -3     
==========================================
- Hits         1071     1061      -10     
- Misses        988      995       +7     

see 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Stygmates
Copy link
Contributor Author

Stygmates commented Sep 6, 2023

Interesting, what was the impact of not having this feature enabled? I would have assumed the WASM CI build step would have failed

I ran into panicked at 'time not implemented on this platform', library/std/src/sys/wasm/../unsupported/time.rs:31:9 that I think was raised when verifying the claims, specifically at this line:

time_fn: Arc::new(Utc::now),
.
I noticed that the implementation of now() used wasn't the correct one (this one which uses the std implementation of now which doesn't support wasm was used instead of this one) because of the feature flag

@ramosbugs
Copy link
Owner

Oh ok, thanks! It looks this will require an update to the MSRV Cargo.lock file to get the 1.65 build to pass:

cp Cargo-1.65.lock Cargo.lock
cargo check
cp Cargo.lock Cargo-1.65.lock

Once that passes, I'll happily merge.

I can also make a PR for oauth2-rs if you want.

That would be greatly appreciated 🙏

@Stygmates
Copy link
Contributor Author

This should pass now, I'll make the PR for oauth2-rs tomorrow, it's 1 AM where I live 😪

@ramosbugs ramosbugs merged commit c42ade0 into ramosbugs:main Sep 7, 2023
6 of 7 checks passed
@ramosbugs
Copy link
Owner

This is now released in 3.3.1

@Stygmates
Copy link
Contributor Author

Thank you for the fast response and fast release of a new version!

ramosbugs added a commit to ramosbugs/chrono that referenced this pull request Apr 9, 2024
std::time::SystemTime::now() panics in WASM environments other than
Emscripten (i.e., wasm32-unknown-emscripten) and WASI (e.g.,
wasm32-wasi). Since compilation errors are preferable to unexpected
runtime panics, this PR removes the `Utc::now()` function from this
crate's public interface altogether in unsupported WASM environments
unless the `wasmbind` feature is enabled. This catches the case in
which a user of the crate forgets to enable the `wasmbind` feature
(see ramosbugs/openidconnect-rs#127 and ramosbugs/oauth2-rs#230) in
build targets that require it.

Fixes chronotope#1301.
ramosbugs added a commit to ramosbugs/chrono that referenced this pull request Apr 9, 2024
`std::time::SystemTime::now()` panics in WASM environments other than
Emscripten (i.e., wasm32-unknown-emscripten) or WASI (e.g.,
wasm32-wasi). Since compilation errors are preferable to unexpected
runtime panics, this PR removes the `Utc::now()` function from this
crate's public interface altogether in unsupported WASM environments
unless the `wasmbind` feature is enabled. This catches the case in
which a user of the crate forgets to enable the `wasmbind` feature
(see ramosbugs/openidconnect-rs#127 and ramosbugs/oauth2-rs#230) in
build targets that require it.

Fixes chronotope#1301.
ramosbugs added a commit to ramosbugs/chrono that referenced this pull request Apr 9, 2024
`std::time::SystemTime::now()` panics in WASM environments other than
Emscripten (i.e., wasm32-unknown-emscripten) or WASI (e.g.,
wasm32-wasi). Since compilation errors are preferable to unexpected
runtime panics, this PR removes the `Utc::now()` function from this
crate's public interface altogether in unsupported WASM environments
unless the `wasmbind` feature is enabled. This catches the case in
which a user of the crate forgets to enable the `wasmbind` feature
(see ramosbugs/openidconnect-rs#127 and ramosbugs/oauth2-rs#230) in
build targets that require it.

Fixes chronotope#1301.
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.

2 participants