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 nested java objects in JSON.stringify #868

Closed

Conversation

tonygermano
Copy link
Contributor

@tonygermano tonygermano commented Apr 16, 2021

I realized that my previous PR #860 did not allow for nested Maps and Lists. Can someone please review to see that I am now creating objects and arrays the correct way? I have a good deal of experience working with java objects in javascript, but very little in the other direction. The new tests are passing.

map.forEach((k, v) -> {
if (k instanceof CharSequence) {
nObj.put(((CharSequence) k).toString(), nObj, v);
nObj.put(((CharSequence) k).toString(), nObj, state.cx.getWrapFactory().wrap(state.cx, state.scope, v, v.getClass()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I be using ScriptRuntime.toObject instead of calling wrap directly? Or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that Context.javaToJS does the right thing more than any of the alternatives I found. I didn't originally choose it because it's only available as a static method, and I already had a Context object, so I didn't think it needed to go through the process of finding the current Context.

@tonygermano tonygermano marked this pull request as draft April 16, 2021 04:34
@tonygermano tonygermano marked this pull request as ready for review April 16, 2021 04:36
@tonygermano
Copy link
Contributor Author

tonygermano commented Apr 16, 2021

I had overlooked until now PR #824, which looks very similar in function to #860 , and also appears to cover the shortcoming I mentioned above. I will take a closer look at it to see how the two approaches might be incorporated and possibly roll back some of my changes.

I think for the time being this PR is still good as it corrects an issue in the current master branch.

- Circular references are now caught
- Enums and some Date and Time types are converted to strings

Co-authored-by: Roland Praml <roland.praml@foconis.de>
@tonygermano tonygermano force-pushed the json-stringify-java-nested branch from 149cbd3 to 7d3985e Compare April 18, 2021 05:38
@gbrail
Copy link
Collaborator

gbrail commented Apr 23, 2021

This looks fine. Since @rPraml (who I can't seem to tag here) wrote the original PR I wonder if he can verify whether it fixes those issues?

Otherwise, since I merged your other PR for this file, can you please resolve the conflicts and merge that one?

@tonygermano tonygermano marked this pull request as draft April 24, 2021 18:59
@tonygermano
Copy link
Contributor Author

Putting this one into draft for now. I'm still considering some other changes after the discussion on his PR.

@tonygermano
Copy link
Contributor Author

Closing this PR in favor of #875

@tonygermano tonygermano deleted the json-stringify-java-nested branch April 29, 2021 00:08
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