Make @webext-core/proxy-service Tree-Shakeable
#111
Conversation
…ync proxy initializers.
…nc what isn't async
…eProxy as async due to dynamic imports. Fix issue with docs compilation.
There was a problem hiding this comment.
Sorry for the slow review, I took some time off from OSS and just getting back today.
This resulted in a 28.4% decrease in the bundled package size just for the demo project.
That's awesome! Thanks for kicking this work off.
I think this is a good start, but I agree that the API you're proposing isn't prefect... I think I know how to work around the top-level await and dynamic import issues. See comment below
| registerMathService(); | ||
| export default defineBackground(() => { | ||
| registerMathService(async () => { | ||
| const { MathService } = await import('../utils/math-service'); | ||
| return new MathService(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is something I'm confused about. I saw in the PR description something about dynamic imports and top-level awaits being required... but that's not true?
You could just import the math-service module here like normal, right? You just can't import it in the entrypoints that use proxies instead.
Here's what I'm thinking, might be a nicer API:
// math-service.ts
export class MathService {}
// background.ts
import { MathService } from '../math-service'; // import the real thing where it is needed
import { registerProxyService } from '@webext-core/proxy-service';
registerProxyService<MathSerivce>('math-service', new MathService())
// content-script.ts
import type { MathService } from '../math-service'; // Import just the types so it's not bundled
import { getServiceProxy } from '@webext-core/proxy-service';
const mathService = getServiceProxy<MathService>('math-service')We could even do something like what Vue does with provide/inject keys and support a custom branded type that let's us infer the service type from the key
// service-proxy-keys.ts
import type { MathService } from '../math-service'; // Important to only import type here
export const ServiceProxyKey = {
MathService: createProxyServiceKey<MathService>('math-service'),
// ...
}
// background.ts
import { MathService } from '../math-service';
import { ServiceProxyKey } from '../service-proxy-keys';
import { registerProxyService } from '@webext-core/proxy-service';
registerProxyService(ServiceProxyKey.MathService, new MathService())
// content-script.ts
import { ServiceProxyKey } from '../service-proxy-keys';
import { getServiceProxy } from '@webext-core/proxy-service';
const mathService = getServiceProxy(ServiceProxyKey.MathService)That way the key and type are tied together.
There was a problem hiding this comment.
Wow, I was very inconsistent in the naming in my suggestions lol. Since the package is called "proxy service", we should stick with that when naming things. I'm OK releasing a major version and straight up dropping the defineProxyService function without any deprecation notice.
So I'm proposing 3 functions:
registerProxyService- takes in a key and actual service implementation, registering messaging listeners for itcreateProxyService- takes in just a key and returns the proxy objectcreateProxyServiceKey- Helper for defining a branded type that the above 2 functions can infer the service type from
This is still in the brainstorming phase, so I'm open for naming changes to any of those APIs. I'm just trying to summarize my suggestions from the last comment.
When I was looking into a specific issue relating to
@webext-core/proxy-service, I noticed #102 that mentioned that, due todefineProxyServicerequiring top-level calls to produce the exports, bundlers can't tree-shake it, causing the service code to be copied into anywhere it's used instead of just the background service.You stated you'd be welcome to a PR refactoring the package, so I tried my hand at this. I'm not sure if it follows your ideas for the package, but it at least follows your contributing standards. Documentation has been updated, tests have been written, and the demo package has been updated. Sorry about the bad commit messages, I was working on this pretty quickly and I've always been bad at commit messages haha.
A new method,
defineServiceProxy, has been created anddefineProxyServicehas been deprecated.defineServiceProxyno longer allows for theinitfunction directly within, but instead requires it within theregisterServicemethod that is returned. This method should also be awaited due to it using promises (This is the part I'm the least thrilled about, top-level awaits are not "great" but utilizing something like wxt should make this easy). You can therefore use things like dynamic imports or asynchronous initializers should they be needed.Let me know your thoughts! If this isn't the direction you're wanting to go with the package, I also completely understand. Just thought I'd try my hand at this issue 😁
Here is the size of the proxy demo before tree-shaking:
And here is the size after tree-shaking:
This resulted in a 28.4% decrease in the bundled package size just for the demo project.