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

Remove support for stdweb #1941

Merged
merged 8 commits into from
May 24, 2021
Merged

Conversation

maroider
Copy link
Member

@maroider maroider commented May 16, 2021

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

We have now had two releases of Winit (0.24 and 0.25) with the stdweb 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 to web-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

@maroider maroider mentioned this pull request May 16, 2021
12 tasks
@maroider maroider added DS - web C - waiting on maintainer A maintainer must review this code labels May 16, 2021
@alvinhochun
Copy link
Contributor

I'm fine with removing stdweb support.

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.

@ryanisaacg
Copy link
Contributor

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?)

@msiglreith
Copy link
Member

I'm fine as well, including removing the feature flags.
Regarding wasm-binding, maybe there will be needs for compatibility with similar libraries (building on top of web-sys) in the future, but it might be possible to get rid of the dependency as well I guess.

@ArturKovacs
Copy link
Contributor

ArturKovacs commented May 17, 2021

I trust the opinion of the experts spoken before me, so I'd say let's move on with this, and also remove the web-sys feature flag in addition to the stdweb one.

@francesca64
Copy link
Member

I also agree with what's been said.

@zicklag
Copy link

zicklag commented May 18, 2021

Sounds good to me. 👍 ( not that I have much of a say or anything. :) )

@ArturKovacs
Copy link
Contributor

@zicklag 😄 that's exactly the kind of energy we need here.

@maroider maroider marked this pull request as ready for review May 20, 2021 20:16
@maroider
Copy link
Member Author

maroider commented May 20, 2021

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.

@ArturKovacs
Copy link
Contributor

I'd say this can be merged, but I think @francesca64 only you wield the power to do so.

@maroider maroider removed the C - waiting on maintainer A maintainer must review this code label May 23, 2021
@francesca64 francesca64 merged commit 657b4fd into rust-windowing:master May 24, 2021
@msiglreith
Copy link
Member

It looks like there are some outdated checks leftover in the branch settings which block the merge process:
grafik

@msiglreith
Copy link
Member

@francesca64 Could you look at the checks above by chance? I assume it requires some changes in the admin settings.

@ArturKovacs
Copy link
Contributor

I guess I should have written this earlier, but note that this was also brought up here #1952 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

9 participants