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

Remove existing plugins mechanism in light of v2 modularisation #1492

Closed
lawrence-forooghian opened this issue Nov 8, 2023 · 14 comments
Closed
Assignees

Comments

@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Nov 8, 2023

See this question from a while ago, and @mattheworiordan’s more recent question here.

One thing that I’m not sure about — as things stand, the v2 modular version of the library is only for web. If we were to replace the plugins mechanism with modules, what would that mean for non-web platforms? Would they always come with delta encoding functionality? Or do we need to extend the modules mechanism to other platforms?

Copy link

sync-by-unito bot commented Nov 8, 2023

➤ Automation for Jira commented:

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

@lawrence-forooghian
Copy link
Collaborator Author

@mattheworiordan @owenpearson I'm going to pick this one up. The thing that's unclear to me is what we do for the default (i.e. non-modular; the one that you get when you import ably as opposed to ably/modules) version of the library. Currently, this version gives you the same functionality that you'd get if you were to create a modular client with all of the available modules. But if we were to introduce a DeltaEncoding module and remove the existing plugins mechanism, this would mean that the default version of the library would now come bundled with delta encoding functionality, increasing its size. Are we OK with that?

@paddybyers
Copy link
Member

This is v2, so it's allowed to be incompatible. What about just not supporting deltas with the monolithic version of the library? The plugins mechanism is no longer supported, so if you want to use deltas you have to use the modular library

@lawrence-forooghian
Copy link
Collaborator Author

That'd be an option. Are you okay with that @owenpearson? And how would you like to handle the case where a v2 user tries to use the existing plugins mechanism (e.g. because they didn’t properly read the migration guide or whatever)? Should we detect it so we can give them some useful information (log a warning / throw an error from the constructor)?

@lawrence-forooghian
Copy link
Collaborator Author

Actually, one problem with the approach that @paddybyers suggested is that as things stand, the modular version only exists for the browser; there isn’t a Node one.

@owenpearson
Copy link
Member

Yeah that is a problem. If we introduced a modular version for node we would be getting into the world of esm vs commonjs in node which is still a world of pain.

Still, there's nothing stopping us from accepting additional params to the DefaultRest/DefaultRealtime constructors in the same way they're accepted by the base clients, right? ie we could just make it so that...

const Ably = require('ably');
const vcdiffPlugin = require('@ably/vcdiff-decoder');

new Ably.Realtime(options, { vcdiffPlugin });

... works the same way as providing the vcdiffPlugin to ClientOptions.plugins? That way it could be provided to the BaseRealtime/BaseRest classes too even if we don't provide those classes in nodejs?

@paddybyers
Copy link
Member

Why not just bundle it in the case of node?

@owenpearson
Copy link
Member

yeah me and lawrence were just discussing this on slack - my comment was more focussed on how to support the ClientOptions.plugins functionality in v2 with the new modular api, i'm all for including it in ably-js

@lawrence-forooghian
Copy link
Collaborator Author

Why not just bundle it in the case of node?

Meaning, Node always comes with vcdiff capability? How do you feel about that, @owenpearson — that is, no changes to public API for non-modular version, and if you want vcdiff on web then you have to use the modular version.

@owenpearson
Copy link
Member

@paddybyers by bundle it you mean actually include it as part of the core library? I do think import size is usually of little concern for node projects so in that sense it's fine but I do think having that asymmetry of what's included in each platform would be hard to document well

@lawrence-forooghian
Copy link
Collaborator Author

This is the approach that I'm currently taking (included in core library on all platforms except web, and on web you have to use the modular version if you want deltas). I think the documentation will pretty much say what I've just written.

@paddybyers
Copy link
Member

Personally I think that's fine, it's the least effort, and consistent with the future direction for modularisation of the browser lib. If in future we decide on a way to modularise the node version, we'd then be able to be fully consistent

@owenpearson
Copy link
Member

OK sure, that's fine with me 👍

lawrence-forooghian added a commit that referenced this issue Nov 28, 2023
A summary of the discussion on #1492:

- the modular API should be the only way to pass optional functionality
  to the SDK

- this means we need to replace the existing ClientOptions.plugins
  mechanism, which is currently used to pass a Vcdiff decoder

- since the modular variant of the SDK only exists for web at the
  moment, we will bundle Vcdiff decoding into all other platforms (in
  which bundle size is not much of a concern)

- on web, if you want deltas, you have to use the modular variant of the
  SDK

So, we remove the ClientOptions.plugins mechanism and introduce a
tree-shakable Vcdiff module, which bundles the vcdiff-decoder library
(meaning that users no longer need to directly import this library).

Note that this means that, currently, it is no longer possible to use
deltas inside a Web Worker. We’ll address this in #1514.

The README example of configuring a channel to use deltas is copied from
the README of the vcdiff-decoder library.

(Once ably-js v2 is released, we should update the instructions in the
vcdiff-decoder library’s README to make it clear they only apply to v1.
I’ve raised #1513 for this.)

Resolves #1492.
lawrence-forooghian added a commit that referenced this issue Nov 28, 2023
I’m going to use them inside `modules.test.js` as part of #1492
(removing the ClientOptions.plugins mechanism).
lawrence-forooghian added a commit that referenced this issue Nov 28, 2023
A summary of the discussion on #1492:

- the modular API should be the only way to pass optional functionality
  to the SDK

- this means we need to replace the existing ClientOptions.plugins
  mechanism, which is currently used to pass a Vcdiff decoder

- since the modular variant of the SDK only exists for web at the
  moment, we will bundle Vcdiff decoding into all other platforms (in
  which bundle size is not much of a concern)

- on web, if you want deltas, you have to use the modular variant of the
  SDK

So, we remove the ClientOptions.plugins mechanism and introduce a
tree-shakable Vcdiff module, which bundles the vcdiff-decoder library
(meaning that users no longer need to directly import this library).

Note that this means that, currently, it is no longer possible to use
deltas inside a Web Worker. We’ll address this in #1514.

The README example of configuring a channel to use deltas is copied from
the README of the vcdiff-decoder library.

(Once ably-js v2 is released, we should update the instructions in the
vcdiff-decoder library’s README to make it clear they only apply to v1.
I’ve raised #1513 for this.)

Resolves #1492.
lawrence-forooghian added a commit that referenced this issue Nov 28, 2023
A summary of the discussion on #1492:

- the modular API should be the only way to pass optional functionality
  to the SDK

- this means we need to replace the existing ClientOptions.plugins
  mechanism, which is currently used to pass a Vcdiff decoder

- since the modular variant of the SDK only exists for web at the
  moment, we will bundle Vcdiff decoding into all other platforms (in
  which bundle size is not much of a concern)

- on web, if you want deltas, you have to use the modular variant of the
  SDK

So, we remove the ClientOptions.plugins mechanism and introduce a
tree-shakable Vcdiff module, which bundles the vcdiff-decoder library
(meaning that users no longer need to directly import this library).

Note that this means that, currently, it is no longer possible to use
deltas inside a Web Worker. We’ll address this in #1514.

The README example of configuring a channel to use deltas is copied from
the README of the vcdiff-decoder library.

(Once ably-js v2 is released, we should update the instructions in the
vcdiff-decoder library’s README to make it clear they only apply to v1.
I’ve raised #1513 for this.)

Resolves #1492.
@lawrence-forooghian
Copy link
Collaborator Author

Done in #1512.

@stmoreau stmoreau changed the title Decide what to do about existing plugins mechanism in light of v2 modularisation Remove existing plugins mechanism in light of v2 modularisation Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants