DEV: Use `type` instead of `method` in ajax calls (PR #8974)

Even though type is an alias for method, we have custom logic in /discourse/lib/ajax that checks only type, and ~200 other ajax calls in the codebase already use type param.

GitHub

You’ve signed the CLA, CvX. Thank you! This pull request is ready for review.

:+1: Look safe, but maybe we should wait after release though? What if some things were relying on the fact that this check was not happening (probably unknowingly) ?

Can we also have a lint rule or similar to prevent us from using method when we should be using type?

What if some things were relying on the fact that this check was not happening (probably unknowingly) ?

I don’t know if they’re relying on it, but definitely using method had quite a few unintended consequences:

  1. It defaulted args.dataType to "json" for all requests instead of just GET
  2. It forced args.cache to true for on all requests instead of just GET (though I think jQ just ignores it for non-GET and non-HEAD calls)
  3. It didn’t update CSRF token when it was missing, before making the call (that’s the reason I had to update the tests: DEV: Use `type` instead of `method` in ajax calls by CvX · Pull Request #8974 · discourse/discourse · GitHub)

None of these are backbreaking (otherwise we would catch that earlier) but yeah, I’d wait with merging after the release. :wink:


Can we also have a lint rule or similar to prevent us from using method when we should be using type?

I thought about it, but soon(-ish) we will most likely migrate to fetch (polyfilled or not) to drop the jQuery dependency, so a custom lint rule would be short-lived. (and fetch has has only one argument for this: method :wink:)

The other way to approach this would be to deprecate or soft-deprecate the old param:

if (args.method) {
  args.type = args.method;
  delete args.method;
}

What do you think?

Bump for @ZogStriP

The other way to approach this would be to deprecate or soft-deprecate the old param:

Sure, that would work :+1:

@CvX any chances you could merge this sooner rather than later?

Can we use the deprecated helper here for consistency?

https://github.com/discourse/discourse/blob/7ec124fc897dced11eaf2e658011cb09b99f25e7/app/assets/javascripts/discourse-common/lib/deprecated.js.es6#L1

And also some short reason for the deprecation would be useful

Looks good, just a couple of small comments

Looks great :slight_smile: