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

Add skip support for newtype enum variants #42

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Add skip support for newtype enum variants #42

merged 1 commit into from
Aug 13, 2024

Conversation

johanholmerin
Copy link

The logic that filtered skipped fields didn't run on newtype variants due to an early return. I added the same logic, which will treat newtype variants with a skipped field as a unit field, and added some tests for different types of enum variants and for internally tagged enums since I couldn't find any.

@siefkenj
Copy link
Owner

@johanholmerin Thanks for the contribution!

It looks like a skipped newtype results in the inner type being undefined rather than null. This makes sense to me. Could you please verify that that is the actual behavior and change the test accordingly?

@johanholmerin
Copy link
Author

The output differed between js & json. I've added missing_as_null as a fix

@siefkenj
Copy link
Owner

JSON doesn't have a null, but undefined becomes null in JSON.

Is having the null important? I think it makes more sense not to have the option and have missing fields be undefined. After all { a: 10 }.b === undefined in Javascript.

@johanholmerin
Copy link
Author

I'm not sure what you mean, JSON has null but not undefined. I used the same code that's used for Style::Unit but I can change it if you have a better suggestion.

@siefkenj
Copy link
Owner

siefkenj commented Aug 12, 2024

I'm not sure what you mean, JSON has null but not undefined. I used the same code that's used for Style::Unit but I can change it if you have a better suggestion.

I mean "{\"foo\": undefined}" is not JSON, but "{\"foo\": null}" is JSON

@siefkenj
Copy link
Owner

Sorry, I didn't read your previous message correctly.

My question is, when is JSON with null being produced via rust code? It is my understanding that if you serialize

struct Foo {
 #[serde(skip)]
 a: usize,
 b: usize
}

it will produce something like {b: 48739} rather than {a: null, b: 48739}

@johanholmerin
Copy link
Author

No worries. I checked the results for both serde_json & serde-wasm-bindgen and you're right about the fields being skipped rather than being null for struct and tuple variants. For the externally tagged newtype it should however be generated the same way as the tuple, i.e. a string. I had assumed it would be enough to change parse_fields to return the same value as for Unit but I actually had to change what style was used. It should work correctly now.

@siefkenj siefkenj merged commit 27d6982 into siefkenj:main Aug 13, 2024
3 checks passed
@siefkenj
Copy link
Owner

Thanks!

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