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.

Sign up for GitHub

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

Jump to bottom

web-sys services conversion #827

Merged
merged 11 commits into from
Jan 15, 2020
Merged

web-sys services conversion #827

merged 11 commits into from
Jan 15, 2020

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Dec 27, 2019 •
edited
Loading

Make services work with web-sys.

Cc #558, #826, #813, #817, #818, #841, #862, #867, #868.

This was referenced Dec 27, 2019
Merged
Merged
Merged
Merged
@daxpedda daxpedda marked this pull request as ready for review December 31, 2019 15:53
@daxpedda
Copy link
Contributor Author

daxpedda commented Dec 31, 2019 •
edited
Loading

I had to do some funny things with an AtomicBool and a mpsc to be able to clean up the memory of created closures.
It works fine, but if somebody has an idea how to do it differently I am all ears.

@daxpedda daxpedda mentioned this pull request Jan 4, 2020
Merged
@jstarry jstarry added this to the 0.12 milestone Jan 6, 2020
daxpedda
daxpedda commented Jan 9, 2020
View reviewed changes
src/services/storage.rs Outdated Show resolved Hide resolved
daxpedda
daxpedda commented Jan 9, 2020
View reviewed changes
src/services/websocket.rs Outdated Show resolved Hide resolved
@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 9, 2020

I was going through the unwraps here, but looking through the JS API all of these calls should not be able to fail, this is just an WebIDL thing as far as I can tell, but I am not sure.

I can still replace all unwraps with expects if it's desired.

jstarry
jstarry reviewed Jan 9, 2020
View reviewed changes
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

I was going through the unwraps here, but looking through the JS API all of these calls should not be able to fail, this is just an WebIDL thing as far as I can tell, but I am not sure.

I can still replace all unwraps with expects if it's desired.

Yeah, it's hard to tell. I was going back and forth on this for append_child. Couldn't find anything that would indicate that it would ever fail. I would err on the side of expressive expects but wouldn't push back if you went with unwraps at your discretion.

I added a comment on the dialog service, window() is not accessible from the worker context so that unwrap should definitely be an expect

src/services/dialog.rs Outdated Show resolved Hide resolved
@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 9, 2020

I would err on the side of expressive expects but wouldn't push back if you went with unwraps at your discretion.

Personally I never use unwraps in my codebases, I was just following the code practices in yew.
So feel free to tell me to change everything to expects if you want to.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 9, 2020

Converted everything to use cfg_if and cfg_match.

jstarry
jstarry reviewed Jan 12, 2020
View reviewed changes
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Great work @daxpedda! I added a few comments and have some requests for handling the fetch and reader services:

  1. I think they should separate the web-sys and stdweb implementations since they are more complex than the other services
  2. I would like those services to be removed from this PR and introduced as separate PRs (one for fetch and one for reader)
  3. In this PR, annotate these two services with stdweb cfg so that the services module compiles on web-sys

Thoughts?

src/services/fetch.rs Outdated Show resolved Hide resolved
src/services/interval.rs Show resolved Hide resolved
src/services/reader.rs Outdated Show resolved Hide resolved
src/services/storage.rs Show resolved Hide resolved
src/services/websocket.rs Outdated Show resolved Hide resolved
src/services/websocket.rs Outdated Show resolved Hide resolved
src/services/websocket.rs Outdated Show resolved Hide resolved
@daxpedda
Copy link
Contributor Author

  1. In this PR, annotate these two services with stdweb cfg so that the services module compiles on web-sys

Not sure what exactly you mean with that.
Is the current solution acceptable?

This was referenced Jan 12, 2020
Merged
Merged
Merged
@jstarry
Copy link
Member

jstarry commented Jan 15, 2020 •
edited
Loading

Not sure what exactly you mean with that.

I'd like to get the web-sys branch to pass CI so was hoping you could add #[cfg(feature = "std_web")] here and here

@jstarry
Copy link
Member

jstarry commented Jan 15, 2020

I would like those services to be removed from this PR and introduced as separate PRs (one for fetch and one for reader)

@daxpedda sorry I meant remove the changes to fetch and reader services. The stdweb implementation can stay, we just need to add #[cfg(feature = "std_web")] in services/mod.rs as I mentioned here: #827 (comment)

@daxpedda
Copy link
Contributor Author

@daxpedda sorry I meant remove the changes to fetch and reader services. The stdweb implementation can stay, we just need to add #[cfg(feature = "std_web")] in services/mod.rs as I mentioned here: #827 (comment)

Ahahaha, misunderstanding galore, gonna revert my work here 🤣.

@jstarry jstarry merged commit c1f621a into yewstack:web-sys Jan 15, 2020
@daxpedda daxpedda deleted the web-sys-services branch January 15, 2020 14:26
@jstarry
Copy link
Member

jstarry commented Jan 15, 2020

Haha 😂 no worries, thanks for fixing so quickly 😄

jstarry added a commit that referenced this pull request Feb 29, 2020
@jstarry @daxpedda
@jplatte @jetli
Add support for building with web-sys (#961)
d38ce42 
* Enable travis

* `web-sys` general conversion (#826)

* Moved patches from different PRs.

* Add bits & pieces and some services.

* Rename `stdweb` feature to `std_web`.

* Move tests and examples to different PR.

* Revert some `cargo_web` handling removal.

* Missed something.

* Implement `console_error_panic_hook`.

* Update Cargo.toml

Co-authored-by: Justin Starry <[email protected]>

* Move document creation to util convenience method (#855)

* `web-sys` listener conversion (#813)

* `web-sys` listener initial try.

* Improve macros?

* Remove generic from `EventListenerHandle`.

* Fix build.

* A cleaner solution?

* Even cleaner.

* Fix `build.rs`.

* Minor improvements.

* Following the yew toml style.

* Fixing visibility.

* Fix `rustfmt`.

* Add `web-sys` re-exports.

* Move general changes to different PR.

* Remove compat.

* Actually remove `compat.rs`.

* Rename `stdweb` feature to `std_web`.

* Move to gloo's `EventListener` and some polish.

* Remove outdated comment.

* Change `EventHandler` to be cancelled on drop.

* `web-sys` html conversion (#817)

* Converting all `html` parts.

* Format.

* Move general changes to different PR.

* Removed compat.

* Rename `stdweb` feature to `std_web`.

* Remove redudant function copy.

* Some polish.

* Move to gloo's `EventListener`.

* Replace `unwrap`s with `expect`s.

* `web-sys` agent conversion (#818)

* Converting `agent`.

* Remove wrong `cfg` in imports.

* Move general changes to different PR.

* Some optimisations.

* Rename `stdweb` feature to `std_web`.

* Fix `ArrayBuffer` to `Uint8Array` conversions.

* Add js module worker.

* Use `cfg-if`` and `cfg-match` to make things clearer.

* Fix `std_web` build.

* Add some polish.

* Add build guards for invalid build configs (#866)

* `web_sys` cfg conversion (#862)

* Use `cfg-if` and `cfg-match` and some polish.

* Mistakes were made.

* Missed line during rebasing.

* Mistakes were undone.

* Remove global.

* Remove part of `global!`.

* `web-sys` services conversion (#827)

* Convert `console`.

* Finish services.

* Some polish.

* Fix `ArrayBuffer` to `Uint8Array` conversions.

* Fix aborting fetch leading to error and some polish.

* Replaced some `unwrap`s with `expect`s.

* Use `cfg_if` and `cfg_match` and do some polish.

* Proper scoping.

* Some fixes.

* Move fetch and reader services to different PR.

* Revert split.

* Fix CI builds (#877)

* Fix derive_props_test

* Move tests (#897) (#898)

* `web-sys` fetch service conversion (#867)

* Split implementation.

* Import global.

* Import global.

* Revert split.

* Make fetch available again.

* Revert "Revert split."

This reverts commit 6e3f101.

* Re-revert split.

* Some polish.

* Move to `wasm_bindgen_futures`.

* Switch to `thiserror`.

* wip

* Update src/services/fetch/web_sys.rs

Co-Authored-By: daxpedda <[email protected]>

* Some more polish.

Co-authored-by: Justin Starry <[email protected]>

* `web-sys` reader service conversion (#868)

* Split reader implementation.

* Revert split.

* Remove leftover files.

* Make reader available again.

* Revert "Revert split."

This reverts commit 8abdc9c.

* Revert "Remove leftover files."

This reverts commit 188c6eb.

* Re-revert split.

* Polish.

* Forgot some part.

* Some polish.

* Some polish.

* `web-sys` examples/tests conversion (#841)

* Fix examples/tests to work with `web_sys`.

* Update `StorageService` usage.

* Split `stdweb` and `web-sys` examples.

* Fixing the shell script.

* Trying to reset file permissions.

* Update to new reader API.

* Update to new fetch API.

* Update to new fetch API.

* Re-enable examples CI.

* Deleted duplicate example.

* Some fixes.

* Fix rand build.

* Fix spawning workers in combination with `wasm-bindgen`. (#901)

* Fix component rendering process (#913)

* wip

* Fix component rendering process

* Simplify yew-macro a bit (#902)

* yew-macro: Simplify Properties validation

* Fix most clippy warnings

* Fix clippy warnings (#912)

* Import Task trait in dashboard example

* Remove duplicate vtag tests

* Fix prevent_default() by non-passive (#958)

* Fix prevent_default() by non-passive

* Fix cargo fmt

* Remove `Option` from most services.

* Remove `Option` from resize service.

* Apply fetch changes.

* Apply reader service changes.

* Fix `node_refs` example.

* Remove web-sys travis branch

Co-authored-by: daxpedda <[email protected]>
Co-authored-by: Jonas Platte <[email protected]>
Co-authored-by: Jet Li <[email protected]>
AlephAlpha pushed a commit to AlephAlpha/yew that referenced this pull request Feb 29, 2020
@jstarry @daxpedda
@jplatte @jetli @AlephAlpha
Add support for building with web-sys (yewstack#961)
29b90f5 
* Enable travis

* `web-sys` general conversion (yewstack#826)

* Moved patches from different PRs.

* Add bits & pieces and some services.

* Rename `stdweb` feature to `std_web`.

* Move tests and examples to different PR.

* Revert some `cargo_web` handling removal.

* Missed something.

* Implement `console_error_panic_hook`.

* Update Cargo.toml

Co-authored-by: Justin Starry <[email protected]>

* Move document creation to util convenience method (yewstack#855)

* `web-sys` listener conversion (yewstack#813)

* `web-sys` listener initial try.

* Improve macros?

* Remove generic from `EventListenerHandle`.

* Fix build.

* A cleaner solution?

* Even cleaner.

* Fix `build.rs`.

* Minor improvements.

* Following the yew toml style.

* Fixing visibility.

* Fix `rustfmt`.

* Add `web-sys` re-exports.

* Move general changes to different PR.

* Remove compat.

* Actually remove `compat.rs`.

* Rename `stdweb` feature to `std_web`.

* Move to gloo's `EventListener` and some polish.

* Remove outdated comment.

* Change `EventHandler` to be cancelled on drop.

* `web-sys` html conversion (yewstack#817)

* Converting all `html` parts.

* Format.

* Move general changes to different PR.

* Removed compat.

* Rename `stdweb` feature to `std_web`.

* Remove redudant function copy.

* Some polish.

* Move to gloo's `EventListener`.

* Replace `unwrap`s with `expect`s.

* `web-sys` agent conversion (yewstack#818)

* Converting `agent`.

* Remove wrong `cfg` in imports.

* Move general changes to different PR.

* Some optimisations.

* Rename `stdweb` feature to `std_web`.

* Fix `ArrayBuffer` to `Uint8Array` conversions.

* Add js module worker.

* Use `cfg-if`` and `cfg-match` to make things clearer.

* Fix `std_web` build.

* Add some polish.

* Add build guards for invalid build configs (yewstack#866)

* `web_sys` cfg conversion (yewstack#862)

* Use `cfg-if` and `cfg-match` and some polish.

* Mistakes were made.

* Missed line during rebasing.

* Mistakes were undone.

* Remove global.

* Remove part of `global!`.

* `web-sys` services conversion (yewstack#827)

* Convert `console`.

* Finish services.

* Some polish.

* Fix `ArrayBuffer` to `Uint8Array` conversions.

* Fix aborting fetch leading to error and some polish.

* Replaced some `unwrap`s with `expect`s.

* Use `cfg_if` and `cfg_match` and do some polish.

* Proper scoping.

* Some fixes.

* Move fetch and reader services to different PR.

* Revert split.

* Fix CI builds (yewstack#877)

* Fix derive_props_test

* Move tests (yewstack#897) (yewstack#898)

* `web-sys` fetch service conversion (yewstack#867)

* Split implementation.

* Import global.

* Import global.

* Revert split.

* Make fetch available again.

* Revert "Revert split."

This reverts commit 6e3f101.

* Re-revert split.

* Some polish.

* Move to `wasm_bindgen_futures`.

* Switch to `thiserror`.

* wip

* Update src/services/fetch/web_sys.rs

Co-Authored-By: daxpedda <[email protected]>

* Some more polish.

Co-authored-by: Justin Starry <[email protected]>

* `web-sys` reader service conversion (yewstack#868)

* Split reader implementation.

* Revert split.

* Remove leftover files.

* Make reader available again.

* Revert "Revert split."

This reverts commit 8abdc9c.

* Revert "Remove leftover files."

This reverts commit 188c6eb.

* Re-revert split.

* Polish.

* Forgot some part.

* Some polish.

* Some polish.

* `web-sys` examples/tests conversion (yewstack#841)

* Fix examples/tests to work with `web_sys`.

* Update `StorageService` usage.

* Split `stdweb` and `web-sys` examples.

* Fixing the shell script.

* Trying to reset file permissions.

* Update to new reader API.

* Update to new fetch API.

* Update to new fetch API.

* Re-enable examples CI.

* Deleted duplicate example.

* Some fixes.

* Fix rand build.

* Fix spawning workers in combination with `wasm-bindgen`. (yewstack#901)

* Fix component rendering process (yewstack#913)

* wip

* Fix component rendering process

* Simplify yew-macro a bit (yewstack#902)

* yew-macro: Simplify Properties validation

* Fix most clippy warnings

* Fix clippy warnings (yewstack#912)

* Import Task trait in dashboard example

* Remove duplicate vtag tests

* Fix prevent_default() by non-passive (yewstack#958)

* Fix prevent_default() by non-passive

* Fix cargo fmt

* Remove `Option` from most services.

* Remove `Option` from resize service.

* Apply fetch changes.

* Apply reader service changes.

* Fix `node_refs` example.

* Remove web-sys travis branch

Co-authored-by: daxpedda <[email protected]>
Co-authored-by: Jonas Platte <[email protected]>
Co-authored-by: Jet Li <[email protected]>
jstarry added a commit that referenced this pull request Mar 1, 2020
@AlephAlpha @jstarry
@daxpedda @jplatte @jetli
Properties 2.0 (#975)
afb5e9d 
* Properties 2.0

* Fix problem when passing a closure to prop_or_else

* Update test names

* Add support for building with web-sys (#961)

* Enable travis

* `web-sys` general conversion (#826)

* Moved patches from different PRs.

* Add bits & pieces and some services.

* Rename `stdweb` feature to `std_web`.

* Move tests and examples to different PR.

* Revert some `cargo_web` handling removal.

* Missed something.

* Implement `console_error_panic_hook`.

* Update Cargo.toml

Co-authored-by: Justin Starry <[email protected]>

* Move document creation to util convenience method (#855)

* `web-sys` listener conversion (#813)

* `web-sys` listener initial try.

* Improve macros?

* Remove generic from `EventListenerHandle`.

* Fix build.

* A cleaner solution?

* Even cleaner.

* Fix `build.rs`.

* Minor improvements.

* Following the yew toml style.

* Fixing visibility.

* Fix `rustfmt`.

* Add `web-sys` re-exports.

* Move general changes to different PR.

* Remove compat.

* Actually remove `compat.rs`.

* Rename `stdweb` feature to `std_web`.

* Move to gloo's `EventListener` and some polish.

* Remove outdated comment.

* Change `EventHandler` to be cancelled on drop.

* `web-sys` html conversion (#817)

* Converting all `html` parts.

* Format.

* Move general changes to different PR.

* Removed compat.

* Rename `stdweb` feature to `std_web`.

* Remove redudant function copy.

* Some polish.

* Move to gloo's `EventListener`.

* Replace `unwrap`s with `expect`s.

* `web-sys` agent conversion (#818)

* Converting `agent`.

* Remove wrong `cfg` in imports.

* Move general changes to different PR.

* Some optimisations.

* Rename `stdweb` feature to `std_web`.

* Fix `ArrayBuffer` to `Uint8Array` conversions.

* Add js module worker.

* Use `cfg-if`` and `cfg-match` to make things clearer.

* Fix `std_web` build.

* Add some polish.

* Add build guards for invalid build configs (#866)

* `web_sys` cfg conversion (#862)

* Use `cfg-if` and `cfg-match` and some polish.

* Mistakes were made.

* Missed line during rebasing.

* Mistakes were undone.

* Remove global.

* Remove part of `global!`.

* `web-sys` services conversion (#827)

* Convert `console`.

* Finish services.

* Some polish.

* Fix `ArrayBuffer` to `Uint8Array` conversions.

* Fix aborting fetch leading to error and some polish.

* Replaced some `unwrap`s with `expect`s.

* Use `cfg_if` and `cfg_match` and do some polish.

* Proper scoping.

* Some fixes.

* Move fetch and reader services to different PR.

* Revert split.

* Fix CI builds (#877)

* Fix derive_props_test

* Move tests (#897) (#898)

* `web-sys` fetch service conversion (#867)

* Split implementation.

* Import global.

* Import global.

* Revert split.

* Make fetch available again.

* Revert "Revert split."

This reverts commit 6e3f101.

* Re-revert split.

* Some polish.

* Move to `wasm_bindgen_futures`.

* Switch to `thiserror`.

* wip

* Update src/services/fetch/web_sys.rs

Co-Authored-By: daxpedda <[email protected]>

* Some more polish.

Co-authored-by: Justin Starry <[email protected]>

* `web-sys` reader service conversion (#868)

* Split reader implementation.

* Revert split.

* Remove leftover files.

* Make reader available again.

* Revert "Revert split."

This reverts commit 8abdc9c.

* Revert "Remove leftover files."

This reverts commit 188c6eb.

* Re-revert split.

* Polish.

* Forgot some part.

* Some polish.

* Some polish.

* `web-sys` examples/tests conversion (#841)

* Fix examples/tests to work with `web_sys`.

* Update `StorageService` usage.

* Split `stdweb` and `web-sys` examples.

* Fixing the shell script.

* Trying to reset file permissions.

* Update to new reader API.

* Update to new fetch API.

* Update to new fetch API.

* Re-enable examples CI.

* Deleted duplicate example.

* Some fixes.

* Fix rand build.

* Fix spawning workers in combination with `wasm-bindgen`. (#901)

* Fix component rendering process (#913)

* wip

* Fix component rendering process

* Simplify yew-macro a bit (#902)

* yew-macro: Simplify Properties validation

* Fix most clippy warnings

* Fix clippy warnings (#912)

* Import Task trait in dashboard example

* Remove duplicate vtag tests

* Fix prevent_default() by non-passive (#958)

* Fix prevent_default() by non-passive

* Fix cargo fmt

* Remove `Option` from most services.

* Remove `Option` from resize service.

* Apply fetch changes.

* Apply reader service changes.

* Fix `node_refs` example.

* Remove web-sys travis branch

Co-authored-by: daxpedda <[email protected]>
Co-authored-by: Jonas Platte <[email protected]>
Co-authored-by: Jet Li <[email protected]>

* Fix vtag test warning (#978)

* Clean up exported apis and doc visibility (#977)

* Clean up exported apis and doc visibility

* Remove unused ScopeHolder

* Change ComponentLink to alias of Scope

* cargo fmt

* update new examples to Properties 2.0

* Fix a mistake when resolving conflicts

* Remove an outdated hack.

Co-authored-by: Justin Starry <[email protected]>
Co-authored-by: daxpedda <[email protected]>
Co-authored-by: Jonas Platte <[email protected]>
Co-authored-by: Jet Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstarry jstarry jstarry left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12
Development

Successfully merging this pull request may close these issues.
2 participants
@daxpedda @jstarry