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

Update SignedApiResponse schema #120

Merged
merged 3 commits into from
Nov 21, 2023
Merged

Conversation

aquarat
Copy link
Contributor

@aquarat aquarat commented Nov 20, 2023

This PR updates the response schema and associated test resources.

Maybe having the type on the output payload dictionary is too verbose 🤷, happy to revert of course, but this way it will be obvious in future if there's a mismatch.

Closes #115

Copy link
Collaborator

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

👍 LGTM. Is there anything more needed?

Maybe having the type on the output payload dictionary is too verbose 🤷, happy to revert of course, but this way it will be obvious in future if there's a mismatch.

Not sure what this means.

@aquarat
Copy link
Contributor Author

aquarat commented Nov 21, 2023

👍 LGTM. Is there anything more needed?

Maybe having the type on the output payload dictionary is too verbose 🤷, happy to revert of course, but this way it will be obvious in future if there's a mismatch.

Not sure what this means.

haha, thanks, if you haven't noticed then it's not a problem 😆
I'm not sure why the build/CI is failing, I'll work on this now.

@aquarat aquarat marked this pull request as ready for review November 21, 2023 09:51
@aquarat aquarat requested a review from Siegrift November 21, 2023 09:51
@aquarat
Copy link
Contributor Author

aquarat commented Nov 21, 2023

I originally included the output type in the handler, but by doing so it caused the dist path to shift up 2-levels, so the path became dist/api/src/index.js instead of just dist/index.js. I reverted the change to prevent this from happening.

Copy link
Collaborator

@Siegrift Siegrift left a comment

Choose a reason for hiding this comment

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

I originally included the output type in the handler, but by doing so it caused the dist path to shift up 2-levels, so the path became dist/api/src/index.js instead of just dist/index.js. I reverted the change to prevent this from happening.

Yeah, they are separate projects (similarly to airnode packages) so to make that work we would need to have commons package which would export these interfaces for both which is too much work.

I think the interfaces should rarely change so this 👍 LGTM.

@aquarat aquarat merged commit 5af2da4 into main Nov 21, 2023
4 checks passed
@aquarat aquarat deleted the 115-fix-signed-response-schema branch November 21, 2023 11:46
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.

Fix signed API response schema
2 participants