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

Better handling of undefined values #1188

Merged
merged 1 commit into from
Dec 9, 2020
Merged

Conversation

pepone
Copy link
Member

@pepone pepone commented Dec 8, 2020

This PR fixes JavaScript OutputStream to better handling undefined values, in general, we treat undefined as not set optional value and null as an empty string or collection, this is not required in the methods that write nonoptional values, allowing these methods to accept undefined with the same semantic as null makes the code more flexible.

The attached file contains a prebuild of the ice npm package that includes this fix and also the previous fix for #1170

ice-3.7.5-pre.zip

Copy link
Member

@externl externl 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. I think we could also just use == instead of === as v == null will also work if v is undefined. However, your way is more explicit.

@pepone
Copy link
Member Author

pepone commented Dec 8, 2020

Looks good. I think we could also just use == instead of === as v == null will also work if v is undefined. However, your way is more explicit.

I prefer the explicit check, I think we have the linter setup to complain about == null, as we don't always want to treat undefined as null.

Copy link
Member

@bentoi bentoi 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!

@pepone pepone merged commit 768a5f7 into zeroc-ice:3.7 Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants