Skip to content

fix: improve event handler types#816

Merged
toddbaert merged 1 commit intomainfrom
fix/flagsChanged-type
Mar 1, 2024
Merged

fix: improve event handler types#816
toddbaert merged 1 commit intomainfrom
fix/flagsChanged-type

Conversation

@toddbaert
Copy link
Member

@toddbaert toddbaert commented Feb 14, 2024

Before this change the flagsChanged prop on EventDetails was of type unknown for all event types, and a cast was needed.

After this change, flagsChanged is ONLY defined on ProviderEvents.ConfigurationChanged, where it should be, as (string[] | undefined) . In other words:

client.addHandler(ProviderEvents.Ready, (details) => {
  expect(details?.flagsChanged?.length).toEqual(1);  // does not compile
});
client.addHandler(ProviderEvents.ConfigurationChanged, (details) => {
  expect(details?.flagsChanged?.length).toEqual(1);  // DOES compile
});

Additionally, flagsChanged does not autocomplete for events handlers other than ProviderEvents.ConfigurationChanged.

Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left one comment :)

@toddbaert
Copy link
Member Author

I'm going to rebase this on #795 when it's merged.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert closed this Mar 1, 2024
@toddbaert toddbaert reopened this Mar 1, 2024
@toddbaert toddbaert force-pushed the fix/flagsChanged-type branch 3 times, most recently from 07e672f to 1ff55a2 Compare March 1, 2024 15:33
@toddbaert toddbaert changed the title fix: define flagsChanged on ConfigurationChanged details fix: improve event handler types Mar 1, 2024
Copy link
Member

@lukas-reining lukas-reining left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@toddbaert toddbaert added this pull request to the merge queue Mar 1, 2024
Merged via the queue into main with commit 33508f1 Mar 1, 2024
@toddbaert toddbaert deleted the fix/flagsChanged-type branch March 1, 2024 17:43
toddbaert added a commit that referenced this pull request Mar 1, 2024
This is an addendum to #816.

Since the `addHandler` overloads for the event types
(`ProviderEvents.ConfigurationChanged` and `NotChangeEvents`) are
mutually exclusive, it was impossible to iterate over them and pass the
same function...

I had to add one more "base" overload that tolerated both. This doesn't
reduce any of the nice type safety added in
#816, but allows users to
iterate over `ProviderEvents` collections as seen in the new tests.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants