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

Upgrade to Realm Core 13.2.0 #5252

Merged
merged 17 commits into from
Feb 3, 2023
Merged

Upgrade to Realm Core 13.2.0 #5252

merged 17 commits into from
Feb 3, 2023

Conversation

kneth
Copy link
Contributor

@kneth kneth commented Jan 12, 2023

What, How & Why?

This closes #5244

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • [ ] 📝 Update COMPATIBILITY.md
  • [ ] 🚦 Tests
  • [ ] 🔀 Executed flexible sync tests locally if modifying flexible sync
  • [ ] 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • [ ] 📱 Check the React Native/other sample apps work if necessary
  • [ ] 📝 Public documentation PR created or is not necessary
  • [ ] 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • [ ] typescript definitions file is updated
  • [ ] jsdoc files updated

@kneth kneth self-assigned this Jan 12, 2023
@cla-bot cla-bot bot added the cla: yes label Jan 12, 2023
@kneth kneth marked this pull request as ready for review February 1, 2023 15:36
lib/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

What specification are we implementing these versions towards? Their value should perhaps match some sort of schema to be useful?

lib/utils.js Outdated Show resolved Hide resolved
src/js_app.hpp Outdated Show resolved Hide resolved
src/android/platform.cpp Outdated Show resolved Hide resolved
deviceVersion = "unknown";
} else if (Platform.OS === "android") {
deviceName = `${Platform.Manufacturer}:${Platform.Brand}`;
deviceVersion = `${Platform.Model}`;
Copy link
Member

Choose a reason for hiding this comment

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

The react native docs on this, describes the Model as:

The end-user-visible name for the Android device.

What is the intended semantics of this property exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The specification split "Samsung GT-I9100" so deviceName is "Samsung" and deviceVersion is "GT-I9100", and I have interpreted deviceVersion as the model.

// RN doesn't support Apple Watch
deviceName = "iPhone";
}
deviceVersion = "unknown";
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to use osVersion here:

Suggested change
deviceVersion = "unknown";
deviceVersion = Platform.osVersion;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found a way to tell me if it is an iPhone 10 or iPhone 11. Platform.osVersion will tell me the version of iOS or Android which is unrelated to the device.

lib/utils.js Outdated Show resolved Hide resolved
Co-authored-by: Kræn Hansen <kraen.hansen@mongodb.com>
Copy link
Contributor

@JacobOscarGunnarsson JacobOscarGunnarsson left a comment

Choose a reason for hiding this comment

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

Aside from Kraens comments I think the changes for the sync connection parameter looks good! Also as we discussed, feel free to change the JS-specific values that are sent over to whatever you think is easiest consumable

Co-authored-by: Kræn Hansen <kraen.hansen@mongodb.com>
@kneth
Copy link
Contributor Author

kneth commented Feb 2, 2023

feel free to change the JS-specific values that are sent over to whatever you think is easiest consumable

I have updated the internal document to reflect the PR.

@kneth kneth merged commit e3331f2 into master Feb 3, 2023
@kneth kneth deleted the kneth/core-13.2.0 branch February 3, 2023 09:07
Comment on lines +157 to +159
if (Platform.isPad()) {
deviceName = "iPad";
} else if (Platform.isTV()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @kneth, these caused a regression since Platform.isPad and Platform.isTV are boolean properties (not methods).

Error getting versions: TypeError: false is not a function

Please see: https://reactnative.dev/docs/platform#ispad-ios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@appden Thank you for reporting. I have a fix in #5491.

@kneth kneth mentioned this pull request Feb 24, 2023
11 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to Realm Core v13.2.0
4 participants