-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[v3] Typed events #4161
base: v3-alpha
Are you sure you want to change the base?
[v3] Typed events #4161
Conversation
Co-authored-by: Fabio Massaioli <[email protected]>
Co-authored-by: Ian VanSchooten <[email protected]>
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 114 files out of 221 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
I'm testing out this branch, and noticed a couple of things:
- nm := e.Data.([]any)[0].(map[string]dnclientd.Network)
+ nm := e.Data.(map[string]dnclientd.Network) I'm glad to do this, but it might be considered a "breaking" change.
|
Hey @IanVS thanks for the testing work!
You're right about both changes and yes, they're breaking. I had forgotten about that detail. I'll have to update the PR and changelog. Let's discuss the details over on Discord. |
The templates all use 5.x
It's now '/dist/plugins'
But keep out of the docs
c2842c7
to
dc90eb4
Compare
dc90eb4
to
2a50cb5
Compare
|
return ok | ||
} | ||
|
||
var knownEvents = map[string]struct{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this generated from events.txt?
"version": "0.24.2", | ||
"resolved": "https://registry.npmjs.org/@esbuild/aix-ppc64/-/aix-ppc64-0.24.2.tgz", | ||
"integrity": "sha512-thpVCb/rhxE/BnMLQ7GReQLLN8q9qbHmI55F4489/ByVg2aQaQ6kbcLb6FHkocZzQhxc4gx0sCk0tJkKBFzDhA==", | ||
"version": "0.21.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a downgrade?
Description
This PR enhances the v3 binding generator with support for registered events with strict data typing. Typescript IDEs will provide autocompletion support in JS/TS files for registered events. Data types will be checked at build time on the frontend, at runtime on the Go side.
🚨 This PR includes one breaking change:
EmitEvent
methods (on App and Window structs) won't wrap event data in a slice anymore when at most one data argument is provided.The change is intended to make
EmitEvent
's behaviour more intuitive for the most common case (one data argument) and align it with the JS runtime.The impact is mitigated by the fact that all runtime type assertions expecting a slice in event data will break, thus alerting developers to the change.
Many thanks to @IanVS who helped with the template updates.
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Examples and automated regression tests have been run and verified to be working correctly. The updated templates have been tested manually.
Test Configuration
Checklist:
website/src/pages/changelog.mdx
with details of this PR