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

Gate js-sys dependency behind 'wasm-bindgen' feature (#496) #499

Merged
merged 1 commit into from Aug 9, 2022

Conversation

xy2i
Copy link
Contributor

@xy2i xy2i commented Aug 5, 2022

This pr fixes the breaking change of time 0.3.12.

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #499 (6b9baa3) into main (ea2d104) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #499   +/-   ##
=======================================
  Coverage   99.45%   99.45%           
=======================================
  Files          70       70           
  Lines        7167     7169    +2     
=======================================
+ Hits         7128     7130    +2     
  Misses         39       39           
Impacted Files Coverage Δ
src/lib.rs 100.00% <ø> (ø)
src/sys/local_offset_at/mod.rs 100.00% <ø> (ø)
src/offset_date_time.rs 99.47% <100.00%> (+<0.01%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@brooksmtownsend
Copy link

brooksmtownsend commented Aug 8, 2022

Hey @jhpratt , just wanted to bump and support this PR, we've been using this library in our Wasm projects for its types and this now compiles in a couple of wbindgen imports that we don't support

35:  (import "__wbindgen_placeholder__" "__wbindgen_describe" (func $_ZN12wasm_bindgen19__wbindgen_describe17h88dc838da8738812E (type 0)))
36:  (import "__wbindgen_placeholder__" "__wbindgen_throw" (func $_ZN12wasm_bindgen16__wbindgen_throw17h1869cd81144be698E (type 10)))
37:  (import "__wbindgen_externref_xform__" "__wbindgen_externref_table_grow" (func $_ZN12wasm_bindgen9externref31__wbindgen_externref_table_grow17h1e8ccfb5b1b49bdbE (type 1)))
38:  (import "__wbindgen_externref_xform__" "__wbindgen_externref_table_set_null" (func $_ZN12wasm_bindgen9externref35__wbindgen_externref_table_set_null17h4a4a592dfc395a0dE (type 0)))

Would love to see this released in a patch version so that our projects can fetch this with cargo update

@stevelr
Copy link

stevelr commented Aug 8, 2022

I'm getting a build error

   |
59 |     pub fn now_utc() -> Self {
   |            -------      ^^^^ expected struct `OffsetDateTime`, found `()`
   |            |
   |            implicitly returns `()` as its body has no tail or `return` expression

The #cfg logic needs to be updated in that function also

@Goorzhel
Copy link

Goorzhel commented Aug 8, 2022

No build errors here but I was wondering why my time-using crate suddenly started pulling in js-sys:

 [[package]]
 name = "time"
-version = "0.3.11"
+version = "0.3.12"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "72c91f41dcb2f096c05f0873d667dceec1087ce5bcf984ec8ffb19acddbb3217"
+checksum = "74b7cc93fc23ba97fde84f7eea56c55d1ba183f495c6715defdfc7b9cb8c870f"
 dependencies = [
  "itoa",
+ "js-sys",
  "libc",
  "num_threads",
  "time-macros",

@xy2i
Copy link
Contributor Author

xy2i commented Aug 8, 2022

I'm getting a build error

   |
59 |     pub fn now_utc() -> Self {
   |            -------      ^^^^ expected struct `OffsetDateTime`, found `()`
   |            |
   |            implicitly returns `()` as its body has no tail or `return` expression

The #cfg logic needs to be updated in that function also

Good catch Steve, thanks :) I updated the #cfg logic and co-authored, let me know if you have issues.

@xy2i xy2i force-pushed the wasmbind-gate branch 2 times, most recently from 6341efb to 2afc738 Compare August 8, 2022 15:31
@jhpratt
Copy link
Member

jhpratt commented Aug 8, 2022

@brooksmtownsend The PR has only been open a couple days. I will review it when I get the chance. I am well aware that this broke some things.

@stevelr
Copy link

stevelr commented Aug 8, 2022

Thanks @xy2iii that latest change works for wasm32-unknown-unknown.

Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

LGTM; merging.

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.

None yet

5 participants