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

Replace std::time::Instant with web_time::Instant #14668

Merged
merged 10 commits into from
Dec 25, 2024

Conversation

cptpiepmatz
Copy link
Contributor

Description

The std::time::Instant type panics in the WASM context. To prevent this, I replaced all uses of std::time::Instant in WASM-relevant crates with web_time::Instant. This ensures commands using Instant work in WASM without issues. For non-WASM targets, web-time simply reexports std::time, so this change doesn’t affect regular builds (docs).

To ensure future code doesn't reintroduce std::time::Instant in WASM contexts, I added a clippy wasm command to the toolkit. This runs cargo clippy with a clippy.toml configured to disallow std::time::Instant. Since web-time aliases std::time by default, the clippy.toml is stored in clippy/wasm and is only loaded when targeting WASM. I also added a new CI job that tests this too.

User-Facing Changes

None.

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

Copy link
Contributor

@132ikl 132ikl left a comment

Choose a reason for hiding this comment

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

lgtm, thanks 🩷

Copy link
Collaborator

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

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

Thanks! Let's land it

@WindSoilder WindSoilder merged commit 4b1f4e6 into nushell:main Dec 25, 2024
16 checks passed
@github-actions github-actions bot added this to the v0.102.0 milestone Dec 25, 2024
@cptpiepmatz cptpiepmatz deleted the web-time branch December 25, 2024 09:53
@fdncred fdncred added the wasm label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants