KartikTalwar / gmail.js

Gmail JavaScript API
MIT License
3.75k stars 457 forks source link

fix: avoid using deprecated jQuery functions to prepare for jQuery 4 … #780

Closed onestep closed 7 months ago

onestep commented 7 months ago

…(#779)

huksley commented 7 months ago

Afaik based on https://blog.jquery.com/2024/02/06/jquery-4-0-0-beta/ invocation of $.each is not deprecated. I tried to analyze gmail.js and as far as I can see, there is only $.isArray() invocation need to be replaced.

Would it make sense to simplify this PR so it only replaces $.isArray with Array.isArray? With such change gmail.js fine with jQuery 4 beta in my local env.

josteink commented 7 months ago

VERY nice work in this PR. This is taking Gmail.js in a general direction I'm very much agreement about, and I'm glad to see someone having the initiative to make it happen. Thank you VERY much!

I've left some comments in the PR code, which I think is worth considering or addressing, but have no doubt, that this is a PR I would like to see landed.

Let's do this!

huksley commented 7 months ago

Btw, regarding the trusted types, as I can understand from the jQuery changes, users of jQuery need to provide "trusted" HTML, i.e. $(el).html(trustedHtmlHereInChromeOnly), jQuery will not do it automatically.

So it needs to be done on the gmail.js side.

josteink commented 7 months ago

Btw, regarding the trusted types, as I can understand from the jQuery changes, users of jQuery need to provide "trusted" HTML, i.e. $(el).html(trustedHtmlHereInChromeOnly), jQuery will not do it automatically.

So it needs to be done on the gmail.js side.

And it needs to be dispatched like that only when in Chrome... Sounds like a new internal helper-method?

And then if we implement that anyway... We might as well not use jQuery for the task at all?

josteink commented 7 months ago

Simply amazing. Very nice!

One slight nitpick left (which is my fault, really)... But apart from that, this is looking great.

That said, this version may not be a drop-in replacement for existing versions without risk of breaking... So based on that I guess this should be a 2.0 release (following SemVer conventions).

Do you want me to push a non-breaking fix for the reply-menu thing we just merged first, or should we take them both in one go? I'm open to both options.

Also: We definitely need a changelog-item for this. You fix? :smile:

josteink commented 7 months ago

Also: Thinking about loud the elimination of jQuery as a dependency completely (and thus from our API), I think it could be achieved in a fairly non-breaking way, by accepting an "output-transformer" type parameter.

Said in TypeScript, roughly like this:

type Transformer<TElem extends HTMLElement, TResult> = (el: TElem) => TResult;
declare class Gmail<Transformer> {
   constructor(private readonly transformer) { .. }
};

const gmailNoJquery = new Gmail(); // implied identity transformer (i => i)
const element = gmailNoJquery.dom.compose().$el; // HTMLElement

const gmailJquery = new Gmail(el => $(el)); // supplied transformer wraps plain HTMLElement into jQuery
const element = gmailJquery.dom.compose().$el; // JQuery

That exact code might not compile, but you get the general idea.

Basically jQuery is something you need to bring if you want it, and if you do, you get to keep 100% the API you used to have.

Edit: To be clear here, this is me thinking ahead. I don't say this needs to be a part of this PR... But if we are breaking compatibility anyway... Maybe we want to add something like this before the next actual release?

onestep commented 7 months ago

Hello @josteink,

I believe that this version does not introduce any breaking API changes, for now it would operate as before - jQuery wrappers would be in place, and if consumer has jQuery object in global scope, it would be used. The only change is that jQuery-less mode is made explicit, but I think it's not a frequent use case.

Let's keep 2.0 for the version with element transformers, or even jQuery-less. 🙂 For now I'd say that this PR is mostly a hotfix for jQuery 4 compatibility.

I've updated constructor argument check order and added a CHANGELOG entry.

onestep commented 7 months ago

Regarding your idea with transformers - let's continue in issue #592 discussion. In general, I think that transformers for input and output and for single/multiple elements would be needed - i.e. dom.inbox_content() returns single element, while dom.inboxes() returns multiple. Also, check.is_peoplekit_compose(el) is assumed to expect el as a single element, while you can pass multiple elements wrapped to $. 🙂 At some point I could think that gmail-js 2 should be a complete jQuery-less spinoff, while gmail-js 1 with jQuery support could stay in security/hotfix maintenance mode for a transition period.

josteink commented 7 months ago

I've made some very small ammendments based on comments from @huksley. I've also tested some of the changes which I can test, and they seem good.

The ones I couldn't test was because of TrustedHTML now being enabled on all accounts I've tried, and that breaking the changed code.

Based on that, I think it is safe to merge this, publishing, and then we can start working on solving the bigger issues.

Thanks for all the work so far, guys!