-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(api): converted bulk trigger to use SDK #7166
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
// eslint-disable-next-line no-restricted-imports | ||
import { NovuCore } from '@novu/api/core'; | ||
// eslint-disable-next-line no-restricted-imports | ||
import { triggerBulk } from '@novu/api/funcs/triggerBulk'; |
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.
cannot do that, it's failing compilation, if importing type the method cannot be called, it's for typescript compilation and intelisense only
const { data: body } = response; | ||
expect(body.data).to.be.ok; | ||
expect(body.data.length).to.equal(3); | ||
const dtoList = bulkTriggerResponse.value.result; |
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 .value.result
duplication is subpar DX. Can we remove the .value
part?
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.
the value belongs to the reilroad programming, the result belongs to the response always being wrapped, we get the body and the headers where the body is in the results, what do you want to do?
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.
Ideally, it should be bulkTriggerResponse.result
and bulkTriggerResponse.error
, right? Do we have leverage to change that on the speakeasy generated code?
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 was resolved offline. We will chat with speakeasy to get flatter responses.
// eslint-disable-next-line no-restricted-imports | ||
import { NovuCore } from '@novu/api/core'; | ||
// eslint-disable-next-line no-restricted-imports | ||
import { triggerBulk } from '@novu/api/funcs/triggerBulk'; |
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.
cannot do that, it's failing compilation, if importing type the method cannot be called, it's for typescript compilation and intelisense only
08951b8
to
fee0563
Compare
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@novu/client
@novu/headless
@novu/framework
@novu/js
@novu/nextjs
@novu/nest
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/react
@novu/react-native
@novu/shared
@novu/stateless
commit: |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
8990b1f
to
336352a
Compare
feat(api): change trigger to use sdk feat(api): pr comments feat(api): converted bulk trigger to use SDK
336352a
to
d56886a
Compare
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer