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

Improve stringify for JObject #1009

Merged
merged 2 commits into from Nov 17, 2021
Merged

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Nov 16, 2021

  • JObjectArray Stringify error #1005 (comment)
  • improves property traversal of JObject fixing serialization
  • still requires custom JTokenConverter in order to map Newtonsoft primitives to correct JS primitives, we just don't know how to handle them otherwise

if (_engine.ClrTypeConverter.TryConvert(key, typeof(string), CultureInfo.InvariantCulture, out var stringKey))
object stringKey = key as string;
if (stringKey is not null
|| _engine.ClrTypeConverter.TryConvert(key, typeof(string), CultureInfo.InvariantCulture, out stringKey))
Copy link
Owner

Choose a reason for hiding this comment

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

Would probably work without the custom IObjectConverter by checking for IConvertible and ultimately for IFormattable if we know we want a string.

Copy link
Owner

Choose a reason for hiding this comment

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

That's how Fluid can handle JToken without configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I just wanted to create a fast path when we already have a string, but might be worth some PR to improve string coercion

@lahma
Copy link
Collaborator Author

lahma commented Nov 16, 2021

@sebastienros OK to merge?

@sebastienros
Copy link
Owner

we just don't know how to handle them otherwise

IFormattable and IConvertible

@lahma
Copy link
Collaborator Author

lahma commented Nov 17, 2021

Did I get it right? Cool stuff, nonetheless.

@sebastienros
Copy link
Owner

It's better than what I do in FLuid ... I might want to do it this way too. Thanks!

@sebastienros
Copy link
Owner

Apparently Convert.ChangeType(Object, Type) will invoke these methods if the object implements IConvertible.

@lahma
Copy link
Collaborator Author

lahma commented Nov 17, 2021

This version doesn't (probably) cause boxing so I guess this is a bit more optimal for these primary types. We don't need to go finding a converter on a second pass when we do this already in our own code. Can I merge or something to improve?

@sebastienros sebastienros merged commit 2d8f1ec into sebastienros:main Nov 17, 2021
@lahma lahma deleted the jobject-stringify branch November 17, 2021 18: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.

None yet

2 participants