Skip to content
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

Relying on jquery #592

Open
SanketDG opened this issue Sep 26, 2019 · 9 comments
Open

Relying on jquery #592

SanketDG opened this issue Sep 26, 2019 · 9 comments

Comments

@SanketDG
Copy link

SanketDG commented Sep 26, 2019

Currently, the library uses a very small subset of jQuery's functionality, but jQuery itself is very huge. The process of using browserify appends jQuery to the source file, which results in a large output file.

Can we somehow take jQuery away from gmail.js?

The primary uses of jQuery in gmail.js are:

  • DOM selection
  • Utility methods like $.each, $.merge, $.param
  • $.ajax

Given we use a somewhat modern flavor of Javascript, we can easily replace these functionalities with native syntax. (maybe some polyfills)

I understand that a CDN can also be used here, which would result in a not so huge file.

@josteink
Copy link
Collaborator

I'm no big fan of jQuery, and I agree that looking only at the internal use in the library, jQuery does not provide a whole lot of value. In a green-field code-development scenario, I would wipe out jQuery in a single commit and move on.

This is however a reasonably mature library now, and people rely on it to keep working.

If we remove use of jQuery, we will change our API-surface and create a breaking change.

I think the only way to get "rid" of jQuery is to create a new "base-object" (Gmail2?) with a new API only.

People would then have the ability to move to the new API without us causing breakage on the old one, until we eventually just obsolete and remove it completely.

That's a long term development process though, and in the meantime we will be shipping effectively double code-size for GmailJS.

So yeah. No perfect solution in sight, and to me, jQuery has not been enough of an issue to be worth all that effort yet.

@onestep
Copy link
Contributor

onestep commented Apr 4, 2023

Hello @josteink,

Recently Google enabled trusted-types CSP in Google Calendar, therefore rendering injected jQuery code unusable, as it relies on innerHTML assignments upon initialization. For now this policy is not enabled for Gmail, but this may change unexpectedly in near future.

jQuery added support for TrustedHTML for 4.0, but it's still in development, and I did not find specific ETA by quick googling.

I think it would be good to transition from jQuery - make it completely optional and introduce el with raw HTMLElements for gmail-js dom() objects. If consumer provides jQuery in Gmail constructor, it would be a reason to provide compatibilty $el wrappers.

What do you think?

@josteink
Copy link
Collaborator

josteink commented Apr 4, 2023

Thanks for a long, informative and reasoned update in this issue.

I think your suggestion sounds quite reasonable.

I’ve always wanted to move away from JQuery, but breaking the API wasn’t something i wanted to do. This provides a real argument for doing so or at least a non-aggressive way to start moving forward.

Thinking in terms of types, I guess we could achieve this with a new instance member/field which acts as a HTMLElement-transformer used to “encode” elements before leaving (or entering?) the api.

This may not compile directly but it should roughly illustrate the idea:

class Gmail<TOutput> {
    
    _transform: (elem: HTMLelement) => TOuptput

    

    dom(name): TOuput {
        const result = someDomNode;
        return this._transform(result); // do this everywhere JQuery is returned today
    }
}

Then one can instanciate a Gmail class with a IdentityTransformer (i => i) to avoid JQuery or instantiate a Gmail with a JQueryTransformer (i => $(i)) to maintain the old API.

That should allow us to change the internals to be JQuery-free and allow JQuery-free usage, while at the same time causing minimal breakage.

What do you think?

@onestep
Copy link
Contributor

onestep commented Apr 5, 2023

Hello @josteink,

This sounds reasonable, and would also work with some other libraries like fabiospampinato/cash, but I think JQueryTransformer could be set automatically if user provides jQuery in counstructor (to keep compatibility), and IdentityTransformer would be used if no jQuery was provided/required.

There are multiple methods which returnJQuery<HTMLElement> according to their signature, like a bunch of DOM element retrievers (dom.inbox_content(), dom.email_subject(), dom.email_body(), etc.) and also some methods expecting jQuery as well as input, like check.is_peoplekit_compose(element), dom.compose(element), dom.email(element). I've tried hacking around some methods, and they start looking like this:

api.check.is_peoplekit_compose = function (el) {
    if (el && typeof el.jquery === 'string') {
        return el.find("div[name=to] input[peoplekit-id]").length !== 0;
    } else {
        return !!el && typeof el.querySelector === 'function' && !!el.querySelector("div[name=to] input[peoplekit-id]");
    }
};

I'm not sure how this could be implemented with element transformer approach. I mean, even if no jQuery module/transformer was provided in constructor, some consumers could still rely on providing jQuery instance to such method and expecting it would work.
If this could be omitted and dropped from contract, probably some utility methods like getElement(), getElements(), isElementPresent() etc. could be helpful in transformer interface to do the DOM lookup job.

@josteink
Copy link
Collaborator

josteink commented Apr 5, 2023

A more complex, but still pretty simple approach could be to have the transformer not just be a function but an object which can transform both in and out.

In that case we can make all incoming calls that (currently) rely on jQuery just instead handle plain DOM elements.

That would probably also work better in a API cyclic kind of way.

That is: Objects retrieved from the API should be usable. That would be nice.

I must admit I haven't looked into how extensive that kind of change would be, not for the API as a whole nor even for single functions, since as you show above ...

For some functions we try to do some sort of input detection.

That said... Usually I like making refactorings in incremental "guaranteed correct" steps.

So if we at first just start with returned values (which should be fairly simple to prove correct)... Then incoming jQuery based parameters that would (kind of) be a dependency brought on the extension on its own through the APIs it decided to use, right?

Meaning the core/internal GmailJs code should still be able to be made trusted-types compatible (if not already).

And after that we could take the step to deal with incoming parameters, if we still think it looks like a problem worth solving.

That's a three-step plan which I kind of believe in.

Or do you perhaps have any other or better suggestion? 🙂

@josteink
Copy link
Collaborator

josteink commented Apr 5, 2023

And for the sake of argument, if we are expecting jquery in api.check.* functions I think it’s fair to treat those as primarily internal for now, and they could be made to:

  1. Warn and/or crash upon detecting incoming jQuery objects
  2. Silently extract first HMLElement from jquery and run regular DOM code afterwards.
  3. Both of the above.

In either case, (external) Jquery-callers will manually need to rewrap the returned value with (their) jquery to keep code working if they expect HTML elements, but jquery supports/encourages that kind of object-agnostic “just in case” code so I don’t think that’s going to cause a lot of outrage.

@josteink
Copy link
Collaborator

Just as a FYI: I'll be busy as heck the upcoming week(s), so if you want to take a stab at this feel free to get going and create a PR when you have something/anything to show for (complete or not).

I'll probably have time to provide feedback if nothing else 😄

@onestep
Copy link
Contributor

onestep commented Feb 29, 2024

Hello @josteink,

Taking into account that jQuery could not be used anymore due to recent TrustedHTML changes in Gmail, would it make sense to avoid using it altogether for DOM manipulations?

@josteink
Copy link
Collaborator

I think changes related to TrustedHTML should be kept in a single issue, to make it easier for everyone to track the conversation.

I'll provide an answer in this thread instead:

#779

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

No branches or pull requests

3 participants