ramosbugs / openidconnect-rs

OpenID Connect Library for Rust
MIT License
427 stars 103 forks source link

Add wasm support #127

Closed Stygmates closed 1 year ago

Stygmates commented 1 year ago

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 .

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

ramosbugs commented 1 year ago

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

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.42% :warning:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/127/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Stygmates commented 1 year ago

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: https://github.com/ramosbugs/openidconnect-rs/blob/414eb35419f84ab370c0d9acc3af72d72b67462c/src/verification.rs#L552. 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 commented 1 year ago

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 commented 1 year ago

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

ramosbugs commented 1 year ago

This is now released in 3.3.1

Stygmates commented 1 year ago

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