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

onestep · 2024-02-29T14:59:55Z

some DOM check functions were rewritten to use native DOM API
jquery 3 dependency was removed from package.json

huksley · 2024-03-01T11:36:03Z

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.

src/gmail.js

josteink · 2024-03-01T11:58:21Z

src/gmail.js

@@ -2378,7 +2374,7 @@ var Gmail = function(localJQuery) {

        // loop through applicable types
        var types = type ? [ type ] : [ "before", "on", "after", "dom" ];
-        $.each(types, function(idx, type) {
+        types.forEach(type => {


If we are replacing $.each with something else, I would rather we replace it with a normal Javascript for..of-iterator if possible.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

Yep, will change.

josteink · 2024-03-01T12:02:00Z

src/gmail.js

@@ -2407,15 +2403,15 @@ var Gmail = function(localJQuery) {
    api.observe.trigger = function(type, events, xhr) {
        if(!events) return false;
        var fired = false;
-        $.each(events, function(action,response) {
+        Object.entries(events).forEach(([action, response]) => {


Not saying this is wrong per se, but how did you determine which call could have $.each be replaced by simple array.forEach() vs in some cases Objects.keys().forEach() and some other cases Objects.entries().forEach(). It seems to me like something which could be a possible source of accidental errors during the conversion.

And a follow up question: Would we avoid those issues if we instead tried to mimick the semantics of the original call by using for..of ?

I've reviewed all changed places to understand the types, and additionally (you've probably noticed 🙂) I've added JSDoc annotation to root object to associate it with d.ts, so IDE may now analyze parameters and properties:

Regarding the follow-up question - for-of works only for iterable objects, and Object is not iterable, so we would still need Object.keys() or Object.entries(), like:

Suggested change

Object.entries(events).forEach(([action, response]) => {

for (let [action, response] of Object.entries(events)) {

Updated. Additionally, updated signatures in type definitions to match expected input.

josteink · 2024-03-01T12:04:21Z

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!

josteink · 2024-03-01T12:11:05Z

src/gmail.js

-            // else leave $ undefined, which may be fine for some purposes.
-        }
+        // leave $ undefined, which may be fine for some purposes.
+        console.warn("GmailJS: jQuery is not provided in constructor parameters, DOM functions may not work.");


I see what you're trying to do here, but I think we want to be even more explicit.

I think this early in the transition, opting out from jQuery should be something that requires intentional effort, and ending up without jQuery is not something which can be done accidentally.

how about....

var Gmail = function(localJQuery) { var $; if (typeof localJQuery !== "undefined") { $ = localJQuery; } else if (typeof jQuery !== "undefined") { $ = jQuery; } else if (typeof localJQuery === "boolean" && localJQuery === false} { // leave $ undefined, which may be fine for some purposes. } else { throw Error("GmailJS requires jQuery to be provided as a constructor argument!"); } }

Then one can create a jQuery-less instance using this syntax:

var gmail = new Gmail(false);

Makes sense. In that case, opting out from using jQuery would be really explicit, so we could even think of returning raw HTMLElements instead of jQuery wrappers.

huksley · 2024-03-01T12:19:58Z

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 · 2024-03-01T13:12:04Z

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 · 2024-03-04T21:11:37Z

src/gmail.js

@@ -17,14 +17,10 @@ var Gmail = function(localJQuery) {
        $ = localJQuery;
    } else if (typeof jQuery !== "undefined") {
        $ = jQuery;
+    } else if (localJQuery === false) {
+        // leave $ undefined, which may be fine for some purposes.


This is my fault really, but this code needs to be tweaked slightly.

We need this check before if (typeof localJQuery !== "undefined") { otherwise this code will never trigged.

josteink · 2024-03-04T21:17:43Z

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

josteink · 2024-03-04T21:36:55Z

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?

…artikTalwar#779) - some DOM check functions were rewritten to use native DOM API - jquery 3 dependency was removed from package.json - updated some function signatures in type definitions

onestep · 2024-03-06T15:28:46Z

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 · 2024-03-06T15:55:11Z

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.

src/gmail.d.ts

Fix editor-config for typescript too.

josteink · 2024-03-07T08:01:48Z

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!

onestep mentioned this pull request Feb 29, 2024

Gmail change for innerHTML #779

Open

josteink reviewed Mar 1, 2024

View reviewed changes

src/gmail.js Show resolved Hide resolved

josteink reviewed Mar 1, 2024

View reviewed changes

onestep force-pushed the gmailjs-779 branch 2 times, most recently from a9c31ac to b6ed6e9 Compare March 4, 2024 18:27

josteink reviewed Mar 4, 2024

View reviewed changes

fix: avoid using deprecated jQuery functions to prepare for jQuery 4 (K…

5bb6cc7

…artikTalwar#779) - some DOM check functions were rewritten to use native DOM API - jquery 3 dependency was removed from package.json - updated some function signatures in type definitions

onestep force-pushed the gmailjs-779 branch from b6ed6e9 to 5bb6cc7 Compare March 6, 2024 16:59

huksley reviewed Mar 6, 2024

View reviewed changes

src/gmail.d.ts Outdated Show resolved Hide resolved

Fixup constructor parameter types.

32e2198

Fix editor-config for typescript too.

josteink merged commit 7cd405b into KartikTalwar:master Mar 7, 2024
3 checks passed

cancan101 mentioned this pull request Mar 13, 2024

Remove unneeded usage of jquery cancan101/email-tracking-extension#509

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

onestep commented Feb 29, 2024

huksley commented Mar 1, 2024

josteink Mar 1, 2024

onestep Mar 4, 2024

onestep Mar 4, 2024

josteink Mar 1, 2024

onestep Mar 4, 2024

onestep Mar 4, 2024

josteink commented Mar 1, 2024

josteink Mar 1, 2024 •

edited

Loading

onestep Mar 4, 2024

onestep Mar 4, 2024

huksley commented Mar 1, 2024 •

edited

Loading

josteink commented Mar 1, 2024

josteink Mar 4, 2024

josteink commented Mar 4, 2024

josteink commented Mar 4, 2024 •

edited

Loading

onestep commented Mar 6, 2024

onestep commented Mar 6, 2024

josteink commented Mar 7, 2024

	Object.entries(events).forEach(([action, response]) => {
	for (let [action, response] of Object.entries(events)) {

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

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

Conversation

onestep commented Feb 29, 2024

huksley commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josteink commented Mar 1, 2024

josteink Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huksley commented Mar 1, 2024 • edited Loading

josteink commented Mar 1, 2024

Choose a reason for hiding this comment

josteink commented Mar 4, 2024

josteink commented Mar 4, 2024 • edited Loading

onestep commented Mar 6, 2024

onestep commented Mar 6, 2024

josteink commented Mar 7, 2024

josteink Mar 1, 2024 •

edited

Loading

huksley commented Mar 1, 2024 •

edited

Loading

josteink commented Mar 4, 2024 •

edited

Loading