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

[SDK-3937] Create a tree-shakable module for realtime publishing #1504

Closed

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Nov 16, 2023

@owenpearson mentioned that we have many browser use cases which only require subscriptions, and no publishing. He suggested that we create a separate tree-shakable module for this functionality.

This commit introduces the API, but the bundle size savings are minimal since it only pulls out the very low-hanging fruit. I think that we could return to this at some point to see what further size savings we could achieve, but I didn’t want to spend too much time on this now.

Resolves #1491.

@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 16, 2023 14:56 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from 72b0574 to b86a1ce Compare November 16, 2023 16:24
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 16, 2023 16:24 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 16, 2023 16:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 16, 2023 16:25 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the remove-static-from-Message-classes branch from cce7de2 to 4ae8580 Compare November 16, 2023 16:25
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from b86a1ce to 57b879d Compare November 16, 2023 16:26
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 16, 2023 16:26 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 16, 2023 16:27 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 16, 2023 16:27 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from 57b879d to d3fd4cb Compare November 16, 2023 16:54
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 16, 2023 16:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 16, 2023 16:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 16, 2023 16:55 Inactive
@lawrence-forooghian lawrence-forooghian marked this pull request as ready for review November 17, 2023 14:35
@lawrence-forooghian lawrence-forooghian changed the title Create a tree-shakable module for realtime publishing [SDK-3937] Create a tree-shakable module for realtime publishing Nov 21, 2023
@lawrence-forooghian lawrence-forooghian force-pushed the remove-static-from-Message-classes branch from 4ae8580 to b59bd66 Compare November 22, 2023 16:16
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from d3fd4cb to ebf5883 Compare November 22, 2023 16:20
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 22, 2023 16:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 22, 2023 16:21 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 22, 2023 16:21 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the remove-static-from-Message-classes branch from b59bd66 to 3b881f8 Compare November 22, 2023 19:24
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from ebf5883 to e54b732 Compare November 22, 2023 19:24
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 22, 2023 19:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 22, 2023 19:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 22, 2023 19:25 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the remove-static-from-Message-classes branch from 3b881f8 to ed5ff7d Compare November 23, 2023 14:43
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from e54b732 to f5a273e Compare November 23, 2023 14:44
@lawrence-forooghian
Copy link
Collaborator Author

Yeah, I think you're right that there's more that can be done. I think the mistake that I made is that I looked at the code and thought "oh, well, even if you're not publishing, you still have to send messages like ATTACH, so most of this stuff to do with sending messages can't be removed", but you're right, ACK and all that stuff only applies to publishing and presence, so I'll take a fresh look.

@lawrence-forooghian lawrence-forooghian force-pushed the remove-static-from-Message-classes branch from ed5ff7d to c09cee7 Compare November 27, 2023 13:03
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from f5a273e to 0081d4f Compare November 27, 2023 13:17
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 27, 2023 13:17 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 27, 2023 13:18 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 27, 2023 13:18 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the remove-static-from-Message-classes branch from c09cee7 to 875282c Compare November 27, 2023 14:29
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from 0081d4f to 79661e0 Compare November 27, 2023 14:30
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 27, 2023 14:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 27, 2023 14:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 27, 2023 14:31 Inactive
Base automatically changed from remove-static-from-Message-classes to integration/v2 November 27, 2023 19:15
Owen mentioned that we have many browser use cases which only require
subscriptions, and no publishing. He suggested that we create a separate
tree-shakable module for this functionality.

This commit introduces the API, but the bundle size savings are minimal
since it only pulls out the very low-hanging fruit. I think that we
could return to this at some point to see what further size savings we
could achieve, but I didn’t want to spend too much time on this now.

Resolves #1491.
@lawrence-forooghian lawrence-forooghian force-pushed the 1491-tree-shakable-publishing branch from 79661e0 to 71eedc2 Compare November 27, 2023 20:21
@github-actions github-actions bot temporarily deployed to staging/pull/1504/features November 27, 2023 20:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/typedoc November 27, 2023 20:26 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1504/bundle-report November 27, 2023 20:26 Inactive
@lawrence-forooghian
Copy link
Collaborator Author

@owenpearson I've pushed another commit which moves the ACK handling into a separate module, which is used by the RealtimePublishing and RealtimePresence modules. The size savings are still tiny, though; it takes BaseRealtime from 102.88KiB on the integration branch down to 100.59KiB.

There is still a bit of msgSerial-related code hanging around in ConnectionManager and MessageQueue, but I think the savings that we'd get from removing it would be even more marginal.

What do you think?

@owenpearson
Copy link
Member

@lawrence-forooghian yeah i'm not really sure if it's worth it tbh - would probably be more convenient for the base client to include publishing than to potential save ~2kb for use cases which don't need realtime publishing :/

@lawrence-forooghian
Copy link
Collaborator Author

@owenpearson should I go ahead and close the issue then?

@owenpearson
Copy link
Member

@lawrence-forooghian yes please 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants