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.

Sign up for GitHub

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

Jump to bottom

fix: Objects wrongly getting converted to Array #2206

Closed
wants to merge 4 commits into from
Closed

fix: Objects wrongly getting converted to Array #2206

wants to merge 4 commits into from

Conversation

mkmandar123
Copy link

@mkmandar123 mkmandar123 commented Jul 5, 2024 •
edited
Loading

Pull Request

Closes: #2198

Approach

Issue demo

@parse-github-assistant Parse GitHub Assistant
Copy link

parse-github-assistant bot commented Jul 5, 2024 •
edited
Loading

Thanks for opening this pull request!

@mkmandar123 mkmandar123 changed the title Json array issue fix: Objects wrongly getting converted to Array Jul 5, 2024
This was referenced Jul 5, 2024
Closed
Merged
dplewis
dplewis suggested changes Jul 5, 2024
View reviewed changes
Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Looks good, just needs some clean up.

src/ObjectStateMutations.ts Show resolved Hide resolved
integration/test/ParseObjectTest.js
@@ -2187,4 +2187,37 @@ describe('Parse Object', () => {
Parse.allowCustomObjectId = false;
});
});

fit('returns correct field values', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove fit, clean up the test and add any additional values you can think of?

Also can you add a unit test to ensure the internals still work this change to JSONArray.
https://github.com/parse-community/Parse-SDK-JS/blob/alpha/src/__tests__/ObjectStateMutations-test.js#L316

Copy link
Member

@mtrezza mtrezza Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please keep fit until you have added and uncommented all the values from #2201 and the test passes.

@mtrezza
Copy link
Member

mtrezza commented Jul 5, 2024 •
edited
Loading

I don't believe this fixes the issue, see #2198 (comment). There is a fundamental question we need to solve. Let's keep discussing it in the issue thread there.

@mkmandar123
Copy link
Author

Looks good, just needs some clean up.

done the cleanup

@mtrezza
Copy link
Member

mtrezza commented Jul 7, 2024 •
edited
Loading

Superseded by #2201; thanks for the help in the investigation @mkmandar123

@mtrezza mtrezza closed this Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrezza mtrezza mtrezza left review comments

@dplewis dplewis dplewis requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Object attributes transformed in array
3 participants
@mkmandar123 @mtrezza @dplewis