-
Notifications
You must be signed in to change notification settings - Fork 161
Deprecate AJAX more quietly #607
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hang on, I forgot this is ext
world. IMO this strays away from an unopinionated facade. Arguably what we deprecated here is this Scala wrapper around Ajax. I'd be 👍 for keeping a vanilla facade for AJAX, but I'll need a little more convincing why we should keep all of this 😅
Oh right, I completely missed that. Well let's just leave it in there to be kind. There's already code out in the wild using it, plus the fact that AJAX isn't deprecated by browsers means there's no reason to deprecate our little helpers around it. Also it never changes so there's no maintenance cost. If browsers deprecated AJAX then I'd be onboard with deprecating it, but there seems to be no reason to remove it either (now that it's in). A users perspective: I'm using it in scalajs-react and now I'm getting warnings that I should use the Fetch API. Like wtf, no, that code works fine, I'll use what I want. I don't have to move to Fetch and it's overreach for scalajs-dom to tell me to do so. |
That's fair. IMHO it should still be deprecated, at the very least to prevent new uses (since we don't offer a similar |
I see what you mean but I don't agree. It's not our job to teach people improvements for their usage for DOM, it's our job to provide DOM facades. Like it would be nice to have in comments, but the reason I disagree with deprecation because it penalises use, and sometimes there are good reasons to use it. There's actually no problem at all using so it feels to me like we'd be forcing our (non-nuanced) opinions on downstream users. |
That's the thing, it's not a facade. If it was, I definitely would agree that it should not be deprecated. Maybe two sides of the same coin, but the wrapper API provided here is an opinion!
Kind of, but just |
Oh crap, I keep forgetting that point. The problem to me is that this isn't a brand new library where we're just guiding new use, my problem is that it creates problems for users who've already used it. It doesn't seem right to me to force them to make unnecessary changes. I guess I'm gonna appeal to your "we'll keep this to the end of time for compatibility" point 😄 I think it's nice to be kind |
Unless I'm missing something, this doesn't create any more problems than all the other deprecations we introduced? Maybe the problem here was the message suggesting to use |
The deprecation warnings are the problem. Having to go around a perfectly-working codebase to sprinkle |
What about the million other deprecation warnings we introduced? 😆 is it just because those are much easier to resolve?
Another motivation is that we're stepping away from Urgh, frustrating but in the end I do agree with you. It's like how we are staying on SJS 1.5 instead of latest and greatest ... technically, there's no reason we have to, but it's about being kind considering our position at the bottom of the hiearchy. |
Personally I consider them legitimate, where as the ext/Ajax stuff I don't. I know you're pointing out valid ways of slicing and dicing it that make the ext stuff invalid too, but it's a bit of a line in the sand to me.
If that's what we're gonna do (and I don't agree that we should) then we should do it holistically, not just to Ajax :)
Yeah I agree. I think it's nicer to just be kind to end-users rather than ticking a tech box which might make us as maintainers feel good. There's so much annoying churn that we have to deal with all the time in tech, it would be nice not to add to it without a good, solid reason imo. |
Oh wait, this is an issue of "legitimacy" and not about convenience? Then we definitely disagree 😆
I thought that's what we did. All of
Yeah, agree ❤️ |
Oh I assumed there'd be more stuff. Damn that changes everything, I was thinking ext/Ajax was an outlier....
What would you think about deferring it's deprecation until 3.x then? |
Yeah it's all semantics at this point, basically we want a deprecation that dissuades new users without annoying existing ones 😆 Let's make an exception. Unlike all of the other deprecations, as you point out this is a deprecation without (easy) replacement which is not so nice. |
Oh hang on, can we use
https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html |
That annotation is only for use within stdlib. private[scala] final class migration(message: String, changedIn: String) extends scala.annotation.ConstantAnnotation |
Bugger, really? Not even in Scaladoc? But it's documented? |
idk, this is the source of truth https://github.com/scala/scala/blob/v2.13.6/src/library/scala/annotation/migration.scala#L30 |
There we go 😁 the deprecation notice renders in the Scaladoc, but doesn't do anything at compile time (consistent with your scalafix not adding it to the API report). |
This still rubs me the wrong way. We have no grounds to say "AJAX is deprecated, use Fetch API instead". So far as I can tell, AJAX is not deprecated. It's not up to us as a facade library to say that it is, when it's not. I think the reason this was deprecated here in scalajs-dom, is to deprecate helpers that aren't facades, right? Is that a decision we made and are sticking too? (I've had some stuff going on this year that's been affecting my memory. Sorry) |
Yes, fair. We can definitely adjust the message. What would you like it to say? Or are you still against deprecation overall maybe?
No worries at all. I thought we did, and personally I'd like to stick to this if we can. Let me see if I can find the relevant discussion. |
What if the message says "This extension/helper/whatever is deprecated, please use |
😆 here you are, blessing
|
This started in #458:
All good, let's just do it. It seems like a ideologically reasonable thing to do. |
OMG I'm so sorry man. I don't want to reveal much but a few types of amnesia (yep there's more than just one) are side-effects of some medical stuff I've been going through. Sorry about that. Let me re-PR this to be clearer as to why it's being deprecated. |
It's really no big deal, don't sweat it. At the time I don't think we appreciated the consequences (we were happily ticking those maintainer boxes as you say 😉) particularly to your own libraries. So I think it's fair game to re-evaluate our position here, especially since this is a deprecation-without-replacement. I think a milder deprecation via scaladoc is reasonable. |
Closes #606