6519: adds trigger events to known names #6698
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6698 +/- ##
=======================================
Coverage 70.18% 70.18%
=======================================
Files 1182 1182
Lines 36806 36815 +9
Branches 6929 6931 +2
=======================================
+ Hits 25831 25840 +9
Misses 10975 10975
☔ View full report in Codecov by Sentry. |
src/types/starterBrickTypes.ts
Outdated
| @@ -24,6 +24,7 @@ import { type Brick } from "@/types/brickTypes"; | |||
| import { type Reader } from "@/types/bricks/readerTypes"; | |||
| import { type Metadata } from "@/types/registryTypes"; | |||
| import { type UnknownObject } from "@/types/objectTypes"; | |||
| import { type StarterBrickType } from "@/starterBricks/types"; | |||
There was a problem hiding this comment.
We may want to/need to move the StarterBrickType type to this file. The idea behind the top-level type's folder to hold types that can be safely imported from anywhere without introducing circular module dependencies
There was a problem hiding this comment.
I didn't see any circular dependency warning, but I'll move it anyway.
| hasDynamicEventName: this._hasDynamicEventName, | ||
| }; | ||
| } | ||
|
|
||
| private visitExtensionPoint( |
|
|
||
| export type EventNameAnalysisResult = { | ||
| /** | ||
| * Statically known event names in the FormState. | ||
| */ | ||
| knownNames: string[]; | ||
| /** | ||
| * Event names that are used by a trigger starter brick. | ||
| * Kept separate for analysis. Combined with knownNames for runtime. |
There was a problem hiding this comment.
NIT: I'm not sure I follow this comment. Could you clarify why they need to be kept separate for the analysis? I'm assuming it's so that different suggestions can be given for Triggers vs. Emitted Events?
Also, IIRC runtime doesn't use this analysis to run bricks. Is the comment referring to which event name are suggested for the Emit Event brick?
There was a problem hiding this comment.
That is what I meant. I updated the comment to be more clear.
| @@ -43,7 +43,14 @@ class CheckEventNamesAnalysis extends AnalysisVisitorABC { | |||
| } | |||
|
|
|||
| get knownEventNames(): string[] { | |||
There was a problem hiding this comment.
The "knownEventNames" vs. "knownNames" distinction is subtle. We probably should improve the naming of knownNames, e.g., knownEmittedNames?
There was a problem hiding this comment.
That's a good point. I'll update the name.
|
When the PR is merged, the first loom link found on this PR will be posted to |
| hasDynamicEventName: this._hasDynamicEventName, | ||
| }; | ||
| } | ||
|
|
||
| private visitStarterBrick( |
There was a problem hiding this comment.
Calling out that I changed the name of this method from visitExtensionPoint
| /** | ||
| * Event names that are used by a trigger starter brick. | ||
| * Kept separate to preserve warnings in the trigger starter brick when the event name is not emitted from another brick. | ||
| * Combined with knownEmittedNames to get all known event names to set in redux. | ||
| * @since 1.8.2 | ||
| */ | ||
| knownTriggerNames: string[]; |
There was a problem hiding this comment.
I'm still unsure if I'm happy with this description.
There was a problem hiding this comment.
Definitely good enough to provide context
grahamlangford
deleted the
6519-modify-event-name-collector-to-look-at-custom-event-trigger-names-configured-for-starter-bricks
branch
What does this PR do?
Demo
https://www.loom.com/share/e0ae7cbd42a74d828071d58961d8916a
Checklist