serversideup / webext-bridge

💬 Messaging in Web Extensions made easy. Batteries included.
https://serversideup.net/open-source/webext-bridge
MIT License
547 stars 50 forks source link

Autocomplete & Error alert extending ProtocolMap as shown in docs in Type Safe Protocols #51

Open noamloewenstern opened 1 year ago

noamloewenstern commented 1 year ago

According to the docs at type-safe-protocols, the way to enable autocomplete and ensure corresponding messages are registered for listeners (and alert ts-error when misused) is to add a shim.d.ts and override the ProtocolMap with custom keys definitions. But, trying to do so, does the following: If I actually set a key in a custom ProtocolMap in a custom shim.d.ts file, then:

The desired behavior is to have a TS type error, when trying to call sendMessage('NonExistingKey').

I've cloned your repo, and overriding the ProtocolMap where it's defined, and then the TS does show error when trying to call a non-existing key. The non-error only accurres when install the package in another project, and trying to customly override the ProtocolMap in an external custom shim.d.ts as the docs recommends.

I've tested this functionality with the most up-to-date version (webext-bridge@next == version 6 beta), in a new project here in order to reproduce the issue using a custom shim.d.ts (I've also updated all the other packages-version, trying to see if the latest TS is the issue - which is not the case...)

Seems that the issue is something to do when installing the package, tthe actual types.d.ts of the package is hardcoded to use the "key" ts-type as a "string", and not a dynamically type check (via "K extends DataTypeKey" etc.) as in the original source code of the 'webext-bridge' package over here. Here is an image of the type when importing the functions:

image


(And on another note, when will the 'next' version be published as the 'latest'?)

zikaari commented 1 year ago

Thanks for reporting, this part of the code was entirely written by another collaborator, I don't have a strong grasp on the how's and what's of this mechanism.

Would you be willing to create a pull request with the proposed changes?

zikaari commented 1 year ago

(And on another note, when will the 'next' version be published as the 'latest'?)

I think it's about time that next becomes the latest. There hasn't been many bug reports on the next so it seems to be okay. Assuming you choose to own this issue (and #52), I'll wait for the patches and release the latest right after it is merged. In case you can't take it on, I'll release the next as-is within next week.

noamloewenstern commented 1 year ago

Thanks for reporting, this part of the code was entirely written by another collaborator, I don't have a strong grasp on the how's and what's of this mechanism.

Would you be willing to create a pull request with the proposed changes?

Sure. hopefully today, maybe within the next few days.

noamloewenstern commented 1 year ago

Ok, seems it's more complicated than simply moving the interface ProtocolMap to an extendable module definition. It creates other type issues in the stream.ts file, and overall complicating types.

If extending the interface via adding a shim.d.ts, it will auto-complete types if the keys are defined inside the shim.d.ts. The issue is that the methods sendMessage and onMessage also allow non-existing keys defined inside the ProtocolMap interface. I think in order to find a a solution, some deep refactorization of the types usage is necessary. I maybe wrong, but after spending some time trying to work around different issues cause by different attempts, I'll leave it for now.