Skip to content

Conversation

@sagardspeed2
Copy link
Contributor

Describe your changes

fix/ensemble-response-mapping

basically assign was overwriting the Response isSuccess getter method and thus it was giving type error

Screenshots [Optional]

Issue ticket number and link

Closes #

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added tests
  • I have added a changeset pnpm changeset add
  • I have added example usage in the kitchen sink app

@sagardspeed2 sagardspeed2 requested a review from evshi December 27, 2024 16:11
@sagardspeed2 sagardspeed2 self-assigned this Dec 27, 2024
@changeset-bot
Copy link

changeset-bot bot commented Dec 27, 2024

🦋 Changeset detected

Latest commit: 079b3b1

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

This PR includes changesets to release 5 packages
Name Type
@ensembleui/react-framework Patch
@ensembleui/react-runtime Patch
@ensembleui/react-kitchen-sink Patch
@ensembleui/react-preview Patch
@ensembleui/react-starter 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

@github-actions
Copy link
Contributor

github-actions bot commented Dec 27, 2024

Visit the preview URL for this PR (updated for commit 079b3b1):

https://react-kitchen-sink-dev--pr969-fix-ensemble-respons-nddx6rls.web.app

(expires Thu, 09 Jan 2025 19:06:35 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6267897ade2ba783b6db70a53a60fc3946d625e9

Copy link
Contributor

@evshi evshi left a comment

Choose a reason for hiding this comment

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

If a request is in flight then isSuccess should be false/undefined

@sagardspeed2 sagardspeed2 force-pushed the fix/ensemble-response-mapping branch from d93998f to f88cd73 Compare January 2, 2025 13:00
Comment on lines 11 to 12
isSuccess?: boolean;
isError?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need to be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This both needs to be optional as we are now just updating the statusCode on API trigger, and isSuccess and isError will update through statusCode by getters

Copy link
Contributor

Choose a reason for hiding this comment

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

You should update the function type to take Partial instead of updating the root interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evshi done

@sagardspeed2 sagardspeed2 merged commit ed2f3a0 into main Jan 3, 2025
3 checks passed
@sagardspeed2 sagardspeed2 deleted the fix/ensemble-response-mapping branch January 3, 2025 18:33
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