KartikTalwar / gmail.js

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

Relying on jquery #592

Open SanketDG opened 5 years ago

SanketDG commented 5 years ago

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:

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 commented 5 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

onestep commented 8 months ago

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 commented 8 months ago

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:

https://github.com/KartikTalwar/gmail.js/issues/779