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

RFC: Tree shaking #1210

Closed
owenpearson opened this issue Apr 25, 2023 · 13 comments
Closed

RFC: Tree shaking #1210

owenpearson opened this issue Apr 25, 2023 · 13 comments
Labels
enhancement New feature or improved functionality.

Comments

@owenpearson
Copy link
Member

owenpearson commented Apr 25, 2023

This RFC proposes the API and internal design for a tree-shakable ably-js client which will be included in version 2 of the library. Tree shaking is a form of dead code elimination used by module bundlers which works by statically analysis of ES6 import statements.

Tree shaking will allow users of the library to import features of ably-js independently, making it possible to import a 'minimal' ably-js client with only the necessary features. Since import size is more of a concern for the web browser environment, this proposal focusses on the ably-js web bundle, although the benefits will still be available to users on other platforms.

Constructor API

For backwards compatibility and convenience, the root import will still be an Ably object containing Rest and Realtime constructors (with all features included):

import Ably from '@ably/web';

const client = new Ably.Realtime(clientOptions);

Additionally, all other fields currently present on the Ably object will be importable directly:

import Message from '@ably/web/message';
import Rest from '@ably/web/message';
import Crypto from '@ably/web/crypto';

A base client will be exported which contains the minimal code for a functional ably-js client.
Other modules will be exported separately and may be passed as in an object as a second argument to the BaseClient constructor:

import BaseClient from '@ably/web/baseclient';
import RealtimeSubscription from '@ably/web/realtimesubscription';
import RealtimePresence from '@ably/web/realtimepresence';

const client = new BaseClient(clientOptions, {
  RealtimeSubscription,
  RealtimePresence,
});

It is possible, using TypeScript generics, to dynamically change the SDK API depending on which fields are defined on the object passed in as the second argument to BaseClient so that, for example, if a user creates a BaseClient with no RealtimePresence then they won't get .presence as an autocomplete option on a RealtimeChannel instance (and of course, TypeScript won't compile code which tries to access such properties).

Modules available for tree-shaking

The ably-js automated bundle size report gives a helpful overview of how the web bundle is currently split in to modules. There are some cases where encapsulation of these modules is incomplete so the numbers are not precise, for example: realtimechannel.ts contains some presence related code, so just subtracting the size of realtime.presence.ts and presencemessage.ts won't give you a precise figure for a client without any realtime presence functionality.

This RFC proposes the following core modules will be available as tree-shakable imports:

  • BaseClient contains auth, http client, utility classes, etc
  • RealtimeChannelSubscription contains classes necessary for realtime channel subscription
  • RealtimePublishing is available separately for publishing realtime messages
    • There will be a large piece of work to untangle realtime publishing from other classes into its own encapsulated module
  • RealtimePresence contains classes necessary for any realtime presence functionality
  • WebSocketTransport is included by default in realtime modules while alternative transports (such as XHRStreamingTransport) are optionally exported separately
    • It should be possible to encapsulate some transport upgrade logic so that it is only imported if additional transports are specified
  • Rest API methods are grouped together in the same way as the SDK API already does (Push, Presence, etc) and all share some modules like PaginatedResource
  • Crypto is available as a separate import
  • JSON is the default encoder andMsgPackEncoder is available as a separate import

Additionally, it is worth considering removing XHRRequest from the base client and making it optional with fetch as the default.

How much code would be eliminated?

A 'minimal' client is defined here as a realtime client which is able to subscribe to messages on a realtime channel. Currently, importing a v1 ably-js costs ~225kb. By inspecting modules from the bundle size report generated by 1ef4028 and calculating the total size of modules which would certainly not be included we would eliminate ~75kb.

Additional savings would come from:

  • Removing code related to realtime publishing
  • Encapsulating transport upgrade behaviour and only including it when additional transports are provided
  • Further encapsulating modules, eg. removing anything presence related from realtimechannel.ts
  • Removal of the callback API and associated argument normalisation code
  • Removing IE and ES3 compatibility code and some soft-deprecated APIs

It's difficult to estimate how significant the savings will be from the above list, but a rough estimate is that the resulting minimal client will be around 100-120kb.

┆Issue is synchronized with this Jira Task by Unito

@sync-by-unito
Copy link

sync-by-unito bot commented Apr 25, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3552

@SimonWoolf
Copy link
Member

I don't fundamentally object to tree-shakability. But honestly the expected savings here seem not that exciting given the additional complexity to the user (and work to us)? 225kB down to 150kB. (Or 100-120 but that's including things like removing IE-compatibility code which would also benefit the non-treeshaked version, so not a fair comparison).

Might be worth flipping the order of things around? Like -- rather than doing tree-shaking first, then doing things like "Removing IE and ES3 compatibility code and some soft-deprecated APIs" to improve things further, why not start with the improvements that'll benefit everyone, then when that's done, reevaluate again to see how much then you'd get with tree-shaking ? ISTM there's quite a bit of heavy baggage in there we can cut quite easily -- eg if we can get rid of all that WordArray stuff now everyone's using browsers that support arraybuffer, that's a good 30kB right there (20kB of cryptojs imports and a bunch more in bufferutils & crypto simplification).

Talking of things to cut -- skimming through the file size history, in 1.2.4 we were at 179kB (minified); we jumped to 193kB with the es6 module conversion, then up to 220kB with the typescript conversion, which seems to have resulted in pages and pages of polyfills being added to the bundle, mostly for language features that we're not using (things like runtime support for transpiled async/awaits). ISTM worth reexamining what settings we transpile with to try and avoid the need for that?

@lawrence-forooghian
Copy link
Collaborator

I was wondering what this proposal would mean for our existing plugins mechanism, which also aims to "avoid excessively bloating the client library". Might we wish to consider removing the plugins mechanism and bringing the VCDIFF plugin into the ably-js codebase as an opt-in module?

@lawrence-forooghian
Copy link
Collaborator

I think that the proposal for splitting the library up into functional areas seems like a sensible one — the exact split I think depends on what our typical customer use cases look like and I guess @mikelee638 is best placed to answer that one.

I agree with @SimonWoolf that in terms of the order in which we do this work (i.e. tree shakability vs other stuff), it would make sense to start with stuff that is lower-hanging fruit and which impacts the largest number of users.

@mattheworiordan
Copy link
Member

mattheworiordan commented May 3, 2023

Thanks for this @owenpearson, there is some great thinking here.

Note: This post has been amended following @abhishiv pointing out my mistake of using Gzip sizes of other SDKs as opposed to raw minified size 🤦

My initial thoughts are:

  • It's interesting to see that .presence wouldn't be available if the Presence class is not included. I was not aware that was possible. This has a bearing on current thinking we're having around how we should support additional capabilities like the Spaces SDKs with our SDKs. See comment thread at https://docs.google.com/document/d/1IpCJL7ME0JavWOjPL0VbsAVOy7nlO-A05H4Lf3vAbqA/edit?disco=AAAAu2535s0. Whilst we don't want to make this work bigger than it should be, rationalising how we deal with composable/shakable SDKs, plugins, and future SDKs for missions, will ensure we don't have all SDKs and teams going in different directions (arguably like we did with AAT).
  • I am surprised at some of the abstractions you're proposing, like RealtimeSubscription. Do we envisage a case where people would use Realtime but wouldn't subscribe or attach to channels? Have we had product input into what feature sets make sense? I think we should get that input so that the features explicitly imported are logical groupings of features.
  • I thought the point of tree-shaking was that the code is "shaken" and any paths to code that is not used is not bundled. This approach appears to be less tree-shaking, and more composability of the SDKs. Have I misunderstood how tree-shaking works with JS bundlers, and is it not possible to have the bundlers automatically detect which components to include based on which code is called?
  • Why is Auth is not being included as a candidate for being split? Most web clients don't need any logic around how to issue Auth tokens etc. and simply get a token / API key, and use that for requests. I imagine there are lots of opportunities to optimise the code so that we have Auth (use tokens and API keys provided) and AuthProvider (issuing tokens etc).
  • I am with @SimonWoolf and @lawrence-forooghian here I am afraid (not meaning to pile in), this work doesn't achieve the outcome we were hoping for, and we should focus on the outcome we want that drove this work being prioritised, over how we thought we'd deliver that outcome. The fact that the work we did, in order to set us up for a smaller web SDK, has now resulted in us having a much larger SDK (22%) bigger IMHO is a problem we need to tackle head on. Speaking with prospects, one of their major gripes continues to be the size of our SDK for simple use cases. I don't have know what the magic KB size would be, but PubNub's is 45KiB 183KiB and Pusher's is 21.5KiB 72.2KiB. We are bigger by between 20% for PubNub and 200% for Pusher. IMHO, we need a more radical plan that gets us within that ballpark at least.
  • As an aside, I think the bundler sizing is a bit misleading as our classes are often polluted with logic from other dependencies, and in addition, we have a lot of "conveniences" in our SDKs that should be stripped when minified. Take innocuous message.ts for example, a large proportion of this code is logic to work with Cipher (because we did not choose to support plugins like we have in Ruby for exmaple), toString methods (convenience), vcDiff specific code, etc. IMHO we should be spending time making our code cleaner and more composable so that we don't leak stuff like this across our classes that makes it near on impossible to optimize.

Happy to have a call to discuss this, especially if we need something more radical, to explore what's possible.

@jaley
Copy link

jaley commented May 10, 2023

I've had a look through the above suggestions and analysis. My 2c:

  • It looks like there's some low-hanging fruit in Simon's suggestions above, which may deliver as much of a reduction in footprint without significant restructuring. For example, I tried locally switching from CommonJS module to ES6 in tsconfig (which, IIUC, just changes what webpack will see?) and that reduced us from 226kb to 202kb. I'm assuming there are other implications to that, but maybe there's further mileage in tuning the compiler and webpack options?
  • I looked at the bundle contents a bit for ably-js as well as the smaller competitor bundles Matt linked above:
    • We seem to have a lot more string literal content than the others. Some quick regexing suggests there's 38KB of string literals in ably.min.js, which is 17% of the bundle size, whereas it's more like 7% for the pusher lib. Some of that is just data we can't drop (like URLs etc), but there's quite a lot of verbose error string literals, which the others seem to be much lighter on. Not sure if there's something we can do there?
    • Pusher at least has no dependencies at all really, just encryption, which is bundled separately.
    • We do have way more code. cloc reports 12k LOC in ably-js, 6k LOC in Pusher, 8k LOC in PubNub. Still, that's a factor of 2x, and as Matt points out above, the difference in bundle size is much more.

@abhishiv
Copy link

abhishiv commented Jun 9, 2023

hey guys

I found this issue since I also don't like the fact that ably is larger compared to pusher/pubnub. But just wanted to say the factor is not 5-10x, since ably is 60kb gziped and pubnub is 45kb.

What I would ideally like is the ably protocol documented somewhere so i can connect to it directly via websocket - they way pusher has documented their protocol.

@mattheworiordan
Copy link
Member

mattheworiordan commented Jun 27, 2023

@abhishiv thanks for pointing out the mistake I made using Gzipped sizes when comparing against our minified byte size 🤦 I've updated my comment now to reflect your comments meaning we're in the ballpark of PubNub now in fact, without all the optimisation work we're doing which is encouraging!

In regards to the protocol documentation, we do in fact document it (it used to be part of our normal docs, but we moved it out as we found most devs weren't interested). See https://sdk.ably.com/builds/ably/specification/main/protocol/ and https://sdk.ably.com/builds/ably/specification/main/features/ for the complete spec.

@VeskeR
Copy link
Contributor

VeskeR commented May 22, 2024

This has been (mostly) implemented in ably-js v2 (too many PRs to link them all, so will mention them when necessary).

We now provide a modular variant of the library available at ably/modular path. We decided to stick with existing plugins mechanism to provide modular functionality for the library.
Core plugins that are available as tree-shakable imports:

  • Rest
  • Crypto
  • MsgPack
  • RealtimePresence
  • WebSocketTransport
  • XHRPolling
  • XHRRequest
  • FetchRequest
  • MessageInteractions
  • vcdiff

Regarding additional savings proposed by Owen in the original message:

  • Removing code related to realtime publishing - ❔ not sure about that, @owenpearson @lawrence-forooghian is this something we did/may want to do in the future (so create a separate issue)?
  • Encapsulating transport upgrade behaviour and only including it when additional transports are provided - 🟡 partially achieved in feat: no upgrade #1645 by removing XHRStreaming and replacing upgrade mechanism with websocket + base fallback transport mechanism
  • Further encapsulating modules, eg. removing anything presence related from realtimechannel.ts - ❌ not done, do we want to create a separate issue for this? @owenpearson @lawrence-forooghian
  • Removal of the callback API and associated argument normalisation code - ✔️ done, public API is now Promise first
  • Removing IE and ES3 compatibility code and some soft-deprecated APIs - ✔️ done, IE support dropped (as well as other obsolete browser version), we target now ES2017, and we removed other deprecated APIs.

crypto-js dependency also has been removed in #1239 as we now use Web Crypto API.

Also, next idea has not been implemented:

It is possible, using TypeScript generics, to dynamically change the SDK API depending on which fields are defined on the object passed in as the second argument to BaseClient so that, for example, if a user creates a BaseClient with no RealtimePresence then they won't get .presence as an autocomplete option on a RealtimeChannel instance (and of course, TypeScript won't compile code which tries to access such properties).

Instead we check available plugins at runtime and throw an error if user tried to use an API that requires a plugin which was not provided. Would we want to export this idea to a separate issue @owenpearson ?


@owenpearson @lawrence-forooghian Once above questions above are resolved, would you be happy to close this issue?

@lawrence-forooghian
Copy link
Collaborator

Instead we check available plugins at runtime and throw an error if user tried to use an API that requires a plugin which was not provided. Would we want to export this idea to a separate issue (…)?

I remember discussing this with Owen during the design of v2 — I think he made an explicit decision to go with the error-throwing approach, so I don't think anything further is needed here.

@lawrence-forooghian
Copy link
Collaborator

Removing code related to realtime publishing

I had a go at this in #1504 and we decided not to proceed with it since the bundle size reduction was disappointing. So I think we can forget about it.

@lawrence-forooghian
Copy link
Collaborator

I think we can close this issue.

@VeskeR
Copy link
Contributor

VeskeR commented Jun 14, 2024

Closing as implemented in ably-js v2.

@VeskeR VeskeR closed this as completed Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improved functionality.
Development

No branches or pull requests

7 participants