Skip to content

Conversation

@vmoroz
Copy link
Member

@vmoroz vmoroz commented Jan 14, 2021

In this PR we are adding JSValue constructors for std::optional<T> and std::nullopt_t.
They are convenient to use with ReactPropertyBag, ReactNotificationService, and other classes that return std::optional<T> which we then want to assign to JSValue.
The std::nullopt_t or empty std::optional<T> will result in creation of JSValue::Null.
For non-empty std::optional<T> the result will depend on the type T.

Microsoft Reviewers: Open in CodeFlow

@vmoroz vmoroz requested a review from a team January 14, 2021 22:40
@vmoroz vmoroz requested a review from a team as a code owner January 14, 2021 22:40
Copy link

@NikoAri NikoAri left a comment

Choose a reason for hiding this comment

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

:shipit:

@vmoroz vmoroz merged commit af70a3e into microsoft:master Jan 15, 2021
inline JSValue::JSValue(double value) noexcept : m_type{JSValueType::Double}, m_double{value} {}
template <class T>
inline JSValue::JSValue(std::optional<T> &&value) noexcept
: JSValue(value.has_value() ? JSValue(std::move(value).value()) : JSValue()) {}
Copy link
Member

Choose a reason for hiding this comment

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

If I understand this correctly this is equating an empty value, with null; shouldn't it be equating it with undefined instead? are null and undefined the same as far as JSValue knows? and if so is that right?

Copy link
Member Author

@vmoroz vmoroz Jan 15, 2021

Choose a reason for hiding this comment

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

The JSValue is designed to represent a JSON value with some design features from folly:dynamic such as having Int64 and Double types instead of a single Number type. It only recognizes Null. It does not support the undefined type.

vmoroz added a commit that referenced this pull request Jan 15, 2021
* Fix unused parameter warning in AbiCallInvoker (#6860)

* Move CallInvoker.h to TurboModule filter folder (#6861)

* Simplify Microsoft.ReactNative.IntegrationTests (#6868)

* Update clang-format version to 1.5.0 (#6870)

* Add JSValue constructor for std::optional<T> (#6873)

* Updated doc comments in IReactNotificationService.idl (#6875)

* Fix ReactNotificationService for notifications between app and modules (#6877)
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.

4 participants