Skip to content

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

Merged
merged 2 commits into from
Oct 20, 2021
Merged

Deprecate AJAX more quietly #607

merged 2 commits into from
Oct 20, 2021

Conversation

japgolly
Copy link
Contributor

Closes #606

@japgolly japgolly requested a review from armanbilge October 18, 2021 02:26
Copy link
Member

@armanbilge armanbilge left a 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 😅

@japgolly
Copy link
Contributor Author

japgolly commented Oct 18, 2021

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.

@armanbilge
Copy link
Member

That's fair. IMHO it should still be deprecated, at the very least to prevent new uses (since we don't offer a similar Future-based wrapper API for fetch, this one actually looks friendlier/more accessible!). But I'd be willing to compromise at "we'll keep this to the end of time for compatibility" or whatever 😆

@japgolly
Copy link
Contributor Author

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.

@armanbilge
Copy link
Member

armanbilge commented Oct 18, 2021

it's our job to provide DOM facades

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!

I disagree with deprecation because it penalises use

Kind of, but just @nowarn?

@japgolly
Copy link
Contributor Author

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

@armanbilge
Copy link
Member

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.

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

@japgolly
Copy link
Contributor Author

The deprecation warnings are the problem. Having to go around a perfectly-working codebase to sprinkle @nowarn everywhere can be more work that it sounds, and it's completely unnecessary. The motivation to deprecate this is... Fetch API is nicer? Maybe AJAX is better/easier for some situations. I don't see the motivation against leaving the old stuff in place.

@armanbilge
Copy link
Member

The deprecation warnings are the problem.

What about the million other deprecation warnings we introduced? 😆 is it just because those are much easier to resolve?

The motivation to deprecate this is... Fetch API is nicer?

Another motivation is that we're stepping away from ext and just focusing on pure facades. But maybe that's more an ideological point.

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.

@japgolly
Copy link
Contributor Author

What about the million other deprecation warnings we introduced?

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.

Another motivation is that we're stepping away from ext and just focusing on pure facades. But maybe that's more an ideological point.

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

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.

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.

@armanbilge
Copy link
Member

Personally I consider them legitimate, where as the ext/Ajax stuff I don't.

Oh wait, this is an issue of "legitimacy" and not about convenience? Then we definitely disagree 😆

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

I thought that's what we did. All of ext is deprecated, what's left?

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.

Yeah, agree ❤️

@japgolly
Copy link
Contributor Author

I thought that's what we did. All of ext is deprecated, what's left?

Oh I assumed there'd be more stuff. Damn that changes everything, I was thinking ext/Ajax was an outlier....

Yeah, agree ❤️

What would you think about deferring it's deprecation until 3.x then?

@armanbilge
Copy link
Member

armanbilge commented Oct 18, 2021

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.

@armanbilge
Copy link
Member

Oh hang on, can we use @migration tag? It seems to be that in-between we are looking for.

@migration like deprecated but provides advanced warning of planned changes ahead of deprecation. Same fields as @deprecated.

https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html

@japgolly
Copy link
Contributor Author

That annotation is only for use within stdlib.

private[scala] final class migration(message: String, changedIn: String) extends scala.annotation.ConstantAnnotation

@armanbilge
Copy link
Member

Bugger, really? Not even in Scaladoc? But it's documented?

@japgolly
Copy link
Contributor Author

@armanbilge
Copy link
Member

armanbilge commented Oct 18, 2021

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

@armanbilge armanbilge changed the title Un-deprecate AJAX Deprecate AJAX more quietly Oct 18, 2021
@japgolly
Copy link
Contributor Author

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)

@armanbilge
Copy link
Member

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.

Yes, fair. We can definitely adjust the message. What would you like it to say? Or are you still against deprecation overall maybe?

I think the reason this was deprecated here in scalajs-dom, is to deprecate helpers that aren't facades, right?

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.

@armanbilge
Copy link
Member

What if the message says "This extension/helper/whatever is deprecated, please use dom.XMLHttpRequest directly instead" ?

@armanbilge
Copy link
Member

armanbilge commented Oct 19, 2021

😆 here you are, blessing ext away!

Yep now that #458 is merged everything in ext is deprecated. chef's kiss

#523 (comment)

@japgolly
Copy link
Contributor Author

This started in #458: Remove all extensions in ext.*

No worries at all. I thought we did, and personally I'd like to stick to this if we can.

All good, let's just do it. It seems like a ideologically reasonable thing to do.

@japgolly
Copy link
Contributor Author

😆 here you are, blessing ext away!

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.

@armanbilge
Copy link
Member

armanbilge commented Oct 19, 2021

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.

@armanbilge armanbilge merged commit a1aeaa3 into master Oct 20, 2021
@armanbilge armanbilge deleted the topic/ajax branch October 20, 2021 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XMLHttpRequest and friends shouldn't be deprecated
2 participants