-
Notifications
You must be signed in to change notification settings - Fork 1k
Remove support for stdweb
#1941
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
Conversation
I'm fine with removing I'm not sure about leaving the feature flag in or not. It would be nice to be able to easily add support for another web binding crate in the future, but as long as there isn't an alternative now it may be more of a burden. |
Personally, I think it's unlikely there will be a new binding crate incompatible with the web-sys interface; I'm not aware of any that exist, other than stdweb (which predates it, is unmaintained, and is generally inferior). My recommendation would be to jettison all of the feature flags, and have the web target specifically and only be compatible with web-sys. This is also a breaking change either way, and should be marked as such. (However, take this with a grain of salt. I bet on stdweb and was wrong; maybe this bet is wrong too?) |
I'm fine as well, including removing the feature flags. |
I trust the opinion of the experts spoken before me, so I'd say let's move on with this, and also remove the |
I also agree with what's been said. |
Sounds good to me. 👍 ( not that I have much of a say or anything. :) ) |
@zicklag 😄 that's exactly the kind of energy we need here. |
CI seems to be mostly happy now, and I think I've made all the required changes to documentation and the like. I'd like to run a sanity-check with a real program later, but this PR should otherwise be ready to be merged. EDIT: Sanity check ran just fine. |
I'd say this can be merged, but I think @francesca64 only you wield the power to do so. |
@francesca64 Could you look at the checks above by chance? I assume it requires some changes in the admin settings. |
I guess I should have written this earlier, but note that this was also brought up here #1952 (comment) |
cargo fmt
has been run on this branchcargo doc
builds successfullyCHANGELOG.md
if knowledge of this change could be valuable to usersWe have now had two releases of Winit (
0.24
and0.25
) with thestdweb
backed marked as deprecated (see #1662 and #1712). Removing it now seems like a reasonable thing to do at this point.There is still some documentation to update, but this is mostly what I'd imagine would be required to remove
stdweb
support from Winit.I do have one thing I'd like to get some opinions on: Should there still be a
web-sys
feature, or should it be unconditionally built now? I don't think there are any viable active alternatives toweb-sys
at the moment, although I know @kettle11 is working on something vaguely in that direction.I know you've stepped down from maintaining Winit, @ryanisaacg, but I'd appreciate it if you had any comments. Similarly, I'd also appreciate any comments from @alvinhochun.
cc @ArturKovacs @francesca64 @msiglreith, as you guys are the remaining maintainers