feat: add public reset API, robust example switching, and contributor metadata #1
Conversation
…velte loader config, and fix example integration
jakmeier
left a comment
jakmeier
left a comment
There was a problem hiding this comment.
Hello @davehorner
Thank you for your interest and contribution!
Are you using or planning to use this crate? I haven't been actively maintaining it and I didn't know anybody using it in their projects. Keep this in mind with regards to the project's maturity.
I have looked through the changes and they generally look great. Getting it updated to current tooling is definitively a plus!
The GIF capture and showing it in the readme is also a nice touch!
I left a few comments and questions, please take a look.
I will also need to check it out and do a few sanity checks, i.e. make sure things work on my machine, too. I will do so once the other details are resolved.
|
|
||
| use wasm_bindgen::prelude::*; | ||
|
|
||
| /// Resets the global div state, allowing re-initialization (for example switching) | ||
| #[wasm_bindgen] | ||
| pub fn reset_global_div_state() { | ||
| // Safety: this is only for example/demo use, not for production | ||
| state::clear_state(); | ||
| } |
There was a problem hiding this comment.
Can you explain this a bit more, please?
Why is it a safety consideration? If so, why is it a hidden comment and not mentioned on the public doc comment?
There was a problem hiding this comment.
The comments need to be removed. A lot of the work of this update was surrounding the initialization of the examples. I don't know that it makes sense to have this in the library or if this should be purely in the associated svelte/js. This is the result of a few different iterations with co-pilot and I didn't put the work in to clean up comments. I usually submit and then coordinate the clean up and deeper analysis. would be interested in hearing your thoughts and I will go back through to spend some more time with the code.
There was a problem hiding this comment.
So the main problem with this function is that it make all existing DivHandle invalid. Using them later can yield unpredictable results.
We could make each handle count its "generations", which changes on a reset. This makes it possible to detect old handles, allowing to return an error when they are used, rather than doing something unpredictable. But I decided against such a design. I wanted to keep it light and simple. So I'd rather not go down that route.
That said, I don't understand why we need this reset function at all. Each example lives in its own div, which is selectively shown or hidden. No need to reset them.
edit: I think I understand now, I'll propose something in a top level comment
Do you think you can rework it to get rid of the reset calls again, like it currently is the case on the master branch?
There was a problem hiding this comment.
if you notice there was also a keyboard closure that was also tracked and reset if I remember.
Ya if you tell me what to do I'll do it. The reset doesn't make sense in the lib but it was the way I went and it did work. Would like it to not reload and you go down this path to support it. it would be better if it was handled in the page.
You are welcome, thank you for sharing this code and for your youtube presentation.
I'm not sure right now. I watched the presentation and thought I would give it a try and it was broken so I wanted to fix it. I spent some time with wasm and I think I got the gist of everything, I wonder if I missed any other things in the repo; I would like to see this crate published on crates.io and see how to actually publish a working app that would integrate the full solution.
Ya unfortunate they deprecated the other library.
Took a little time to get that going but it produced a pretty nice result.
I will follow.
for sure. look forward to hearing it works! |
|
Take a look. |
- Updated example crates (hello_svelte, reposition, styled, toggle) for improved state management and reset logic. - Modified `src/pane.rs` and example source files for better robustness and compatibility. - Updated Webpack and package configuration in `examples/www` for build improvements. - Improved documentation in `README.md`.
|
|
||
| #[wasm_bindgen(start)] | ||
| pub fn main() { | ||
| // Enable better panic messages |
There was a problem hiding this comment.
Yes, please remove this here and also remove the added dependency in this example
|
@davehorner I've tested it locally. I'm afraid the loading is a bit broken still. It seems when I start it, at first it shows me all examples at once, overlapping each other. I think the manual resetting causes this. As I wrote in another comment, I don't think any resetting is necessary. It worked just fine before. Looking into it some more, I realized you completely changed how the example page works. It no longer reloads the page when switching between examples. So, we end up with multiple div-rs WASM instances on the same page. That's not how div-rs is supposed to be used and will always make problems. I assume you did that change to make the gif capture easier? That's neat. But submitting it combined like this makes it hard to review and maintain. Here is my suggestion: Can you split your changes in two separate PRs? One that just updates the infrastructure to work with modern webpack and without webstd? Ideally without any changes on the examples. Or only those that really are necessary. This should be relatively easy to get done, without any breaking changes. In a second PR, you may add the gif capture feature to the examples page. But honestly, you would probably need to start from scratch, doing it in a div-rs compatible way. That is, without loading multiple div-rs instances on the same page. And ideally without changing the public API of the crate, unless it is useful and clean outside the use-case for examples. The first of the two PRs would be a great contribution to the crate! If you can land that, I'm happy to release a new version on crate.io with that. The second part is a bit more optional from my point of view. I will review it and support you where needed, if you want to try and make it work. But if this was just an experimentation and you have enough of it now, we can also drop that change. |
| // Prevent default form submission (which reloads the page) | ||
| form.addEventListener('submit', function(event) { | ||
| event.preventDefault(); | ||
| }); |
There was a problem hiding this comment.
Reloading the page was intended and is important to switch between the different WASM instances of each example. They each bundle their own div-rs instance. You can probably make it work if you know what you are doing. But I can see how this might have caused some problems and why you went down the resetting route.
There was a problem hiding this comment.
ya I didnt like the reload, if that is what you intended that's fine too.
|
I just wanted an image. the gif stuff can go, it was more fun to do that than to download an app. :)
I would like to see the crate expose a binary that would work on the users machine when installed. the last mile of this is still not clear to me. Sometime this week I hope to get back to this and think about it some more. Thanks for the feedback. |
I saw that when it was broken when I first tried the project, that is how it presented itself. I do not see that anymore...I will look. |
Okay cool! Yeah the picture we can still add in the Readme, it's a nice addition.
What do you mean by a binary? The crate itself is only useful as a library. And the example can be run locally with npm. Not exactly a binary but pretty much the equivalent of it in the node / js world. If you explain what exactly it is you want, I can try and help.
Sure, sounds great! I'm looking forward to it. |
|
I'd like to see the example as a binary that can run and establish the needed deps and actually produce a running example. Actually ship an exe that enables using it without being a developer. Maybe I'm asking for too much with too many parts |
|
Okay I see now, thanks for clarifying.
This is a web project. It only makes sense to run it in the browser. You can't run an exe in the browser.
Why would anyone who isn't a developer checkout this repository and try to run an exe in it? This is a library for web + rust development. Examples are directly targeted at developers in that category, so they can learn how to use the crate in their own project. I really don't understand the point of making a development library accessible to non-developers. Even inexperienced devs can follow the step by step explanations on how to run examples. |
|
maybe it doesnt belong in this library and maybe I'm not understanding. if I write an app and take on this dependency, I'm not sure how to distribute it as a whole. |
|
You distribute it like any other web project. By using a web server that serves your files through HTTP(s). Node includes a web server. With Usually, you would put the web server on a host machine somewhere in a data center (a.k.a. in the cloud) which has a public ip. And then you purchase a domain and point that to your public ip through the DNS. If you are not familiar with this level of how the web works, I highly encourage you to educate yourself on the basics. There is plenty of educational material out there on web dev. Courses, books, etc. Or even just asking the right questions to an LLM, if you prefer. Or if you just want to get something done without learning the basics required to become a web developer, search for "how to publish a node project" and you will find dozens of step-by-step guides using different tech stacks. However, none of that is in the scope of |
|
I appreciate the response. I'm not completely lost. I've been around the block. I'd like to automate a standalone app that stands this all up locally. like a tauri app. idk, I don't mean to complicate this PR, its just an aside. It isn't in scope. |
|
I see. Yeah, using it in a tauri app could be an interesting project. But yeah, out of scope here. |
2 participants
This PR introduces several improvements and fixes across the div-rs example suite and core library:
Adds a public reset_global_div_state() API to the Rust library, allowing robust, reload-free example switching.
Updates all example crates (styled, toggle, etc.) to use the new reset API and improve state cleanup.
Refactors example logic for safer event handling, state management, and DOM cleanup.
Improves the JS example loader with better switching, code highlighting, and UI polish.
Adds David Horner as a contributor in package.json and as an author in all changed example Cargo.toml files.
Updates build tooling and configuration (Webpack, Svelte loader, CopyWebpackPlugin).
Adds code highlighting and word wrap for code blocks in the example UI.
General code cleanup and documentation improvements.
These changes make the example suite more robust, maintainable, and contributor-friendly, and ensure proper credit for all contributors.
chore: migrate to modern WASM/JS toolchain and update examples
This PR modernizes the build and example infrastructure for the div-rs project:
Migrates all examples and build scripts to use Webpack 5 for native async WebAssembly support.
Removes all usage of stdweb in favor of web-sys, wasm-bindgen, and (where needed) gloo-timers for browser interop.
Updates Svelte loader and Webpack config for Svelte 3+ compatibility, including resolve.conditionNames.
Refactors example Rust code to use modern web-sys APIs and idioms.
Updates and simplifies npm scripts for building, cleaning, and running examples.
Upgrades all JS dependencies and devDependencies to current versions.
Adds a "Notable Changes" section and improved setup instructions to the README.
Fixes minor bugs and improves code clarity in core and example logic.
These changes bring the project up to date with the latest Rust/WASM and JS ecosystem best practices, making it easier to maintain and extend.