Skip to content
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

fix(kno-7843): fixes trigger_data params functionality and types #428

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

MikeCarbone
Copy link
Contributor

@MikeCarbone MikeCarbone commented Feb 24, 2025

fix(kno-7843): consistent trigger_data params functionality

Note: this PR is a branch off the React 19 updates branch. I wanted to make sure the comparison logic I changed would work with the new zustand updates


This PR addresses KNO-7843, an issue with the SDK where specifying a trigger_data field when using useNotifications() would require stringifying booleans, and would create an awkward typing situation. It didn't work out of the box.

As specified by our API, trigger_data field should be a string. Our SDK's typing says it should be an object, which is correct for a nice DX. This PR remediates those differences elegantly so that our usage is consistent, and if a user (like the one Matt highlighted in the issue) did an override, their code won't break on them and will respect that.


Example 1: supports existing overrides. Users might have done this to get things working and we don't want that to break

const feedClient = useNotifications(
  knockClient,
  process.env.NEXT_PUBLIC_KNOCK_FEED_CHANNEL_ID!,
  {
    tenant,
    // @ts-expect-error need to send a string to the API
    trigger_data: JSON.stringify({ isEnterprise: true }),
  },
);

Example 2: supports proper JS object usage, as expected

const feedClient = useNotifications(
  knockClient,
  process.env.NEXT_PUBLIC_KNOCK_FEED_CHANNEL_ID!,
  {
    tenant,
    trigger_data: { isEnterprise: true },
  },
);

Example 3: supports stringified keys

const feedClient = useNotifications(
  knockClient,
  process.env.NEXT_PUBLIC_KNOCK_FEED_CHANNEL_ID!,
  {
    tenant,
    trigger_data: { "isEnterprise": true },
  },
);

Example 4: and of course, supports no trigger data usage at all

const feedClient = useNotifications(
  knockClient,
  process.env.NEXT_PUBLIC_KNOCK_FEED_CHANNEL_ID!,
  { tenant },
);

Example 5: Tightening trigger_data type (errors)

The API won't accept a nested object when passed through the trigger_data field (422 invalid params), so I tightened up that type to error for users when they try to do that.

Example error:

Screenshot 2025-02-24 at 6 43 46 PM

API docs on trigger data

Note: Adds deep-equal dependency

When using a nested key structure like the examples above, zustand's shallow function within useStableOptions was no longer doing what we needed it to do. The shallow copy would not safely recognize the nested key structure, causing an infinite re-render loop. The deep-equal usage fixes this. I'm not sure how this would work before!


Linear ticket
Loom example of the fix

@MikeCarbone MikeCarbone added the bug Something isn't working label Feb 24, 2025
Copy link

linear bot commented Feb 24, 2025

Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
javascript-ms-teams-connect-example 🛑 Canceled (Inspect) Feb 24, 2025 11:59pm
javascript-nextjs-example ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 11:59pm
javascript-slack-connect-example ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 11:59pm
javascript-slack-kit-example ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 11:59pm

Copy link

changeset-bot bot commented Feb 24, 2025

🦋 Changeset detected

Latest commit: 504c5fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@knocklabs/client Patch
@knocklabs/react-core Patch
client-example Patch
@knocklabs/expo Patch
@knocklabs/react-native Patch
@knocklabs/react Patch
@knocklabs/expo-example Patch
@knocklabs/react-native-example Patch
nextjs-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@MikeCarbone MikeCarbone changed the title fix(kno-7843): consistent trigger_data params functionality fix(kno-7843): fixes trigger_data params functionality and types Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale PR needs to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant