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 toJsonObject export #75

Merged
merged 2 commits into from
Sep 11, 2024
Merged

fix toJsonObject export #75

merged 2 commits into from
Sep 11, 2024

Conversation

fat23cat
Copy link
Contributor

The original @aws-appsync/utils package allows to import toJsonObject in two ways:

import { toJsonObject } from '@aws-appsync/utils/rds';

const result = toJsonObject(ctx.result);
import { util } from '@aws-appsync/utils';

const result = util.rds.toJsonObject(ctx.result);

The change allows to import toJsonObject in both ways as it does the original package

@simonrw simonrw self-requested a review September 11, 2024 13:37
@simonrw
Copy link
Collaborator

simonrw commented Sep 11, 2024

Hi, thanks for raising this PR!

I would normally ask you to add an integration test, however this functionality requires changes in the test execution plumbing. It's a little hacky and needs tidying up.

(If you are interested in further contributions, the test infrastructure code could do with a review from someone more familiar with JavaScript than me! 😜 )

Are you ok if I push a commit updating the tests to cover this change?

@fat23cat
Copy link
Contributor Author

Are you ok if I push a commit updating the tests to cover this change?

Hi! Sure, am ok with it!

@simonrw simonrw self-assigned this Sep 11, 2024
Copy link
Collaborator

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

Also: note the localstack-test test is expected to fail for forks, since forks don't have access to the repository secrets that we need. We probably need to think about how to handle that case.

Thanks for making this change!

@simonrw simonrw merged commit f093444 into localstack:main Sep 11, 2024
3 of 4 checks passed
@fat23cat fat23cat mentioned this pull request Sep 11, 2024
simonrw pushed a commit that referenced this pull request Sep 12, 2024
I'm **really** sorry I made a mistake in the import in the #75 :(
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.

2 participants