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

Nested unmanaged types #6943

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

liamjones
Copy link

@liamjones liamjones commented Dec 6, 2024

What, How & Why?

Ensures Unmanaged works when a Realm.Object contains other Realm.Objects (or Lists, etc of them).

This closes #6037

☑️ ToDos

  • 👨‍💻 Expand checks to existing things that already fully work; Date, UUID, etc
  • ❓ Why did the Counter PR add ExtractPropertyNamesOfTypeExcludingNullability instead of using ExtractPropertyNamesOfType?
  • 👨‍💻 Expand checks to include things that currently half work; List, Dictionary, Set, Counter so tests fail
  • 👨‍💻 Update types so that fully supports the existing half-working things and tests pass again
  • 👨‍💻 Expand checks to include things that don't currently work; nested Object, List, etc so tests fail
  • 👨‍💻 Update Unmanaged so that it supports the new things and tests pass again
  • ❓ Anything extra needed for embedded?
  • ❓ Anything extra needed for linking objects?
  • ❓ Should type:tests be run as part of a higher level build or test?
  • ❗ String still accepted as values, matches Unmanaged<DefaultObject, never>?
  • ❓ If a Realm.Object has required types shouldn't they be automatically required in the Unmanaged version during realm.create() too? Is this possible or will there be a circular type reference?
  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 📝 Update COMPATIBILITY.md
  • 🚦 Tests
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 💥 Breaking label has been applied or is not necessary
  • 🔔 Mention @realm/devdocs if documentation changes are needed

Copy link

cla-bot bot commented Dec 6, 2024

Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @liamjones. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a @cla-bot check comment. The GitHub usernames you file there will need to match that of your Pull Requests. If you have any questions or cannot file the CLA electronically, make a comment here and we will be happy to help you out.

@liamjones
Copy link
Author

@cla-bot check

@cla-bot cla-bot bot added the cla: yes label Dec 6, 2024
Copy link

cla-bot bot commented Dec 6, 2024

The cla-bot has been summoned, and re-checked this pull request!

@liamjones
Copy link
Author

Hi @kraenhansen,

This PR has been slow going due to other work cropping up but I thought I might as well get a draft PR up so I can start discussing bits of it with the team.

There's a couple of TODOs you might be able to help with immediately:

I've got type:tests working again in a sensible manner (I think?): b0be346. The question; should type:tests be run as part of a higher-level build or test command rather than only being run directly? Maybe it should just be an extra pr-lint check command?

The second query, and this might be one for the original PR author instead, do you know why the Counter PR added ExtractPropertyNamesOfTypeExcludingNullability instead of using ExtractPropertyNamesOfType? It wasn't immediately obvious to me.

@liamjones
Copy link
Author

liamjones commented Dec 11, 2024

Unfortunately, I won't have time to finish this properly right now.

I'm stuck with an issue whereby RequiredProperties in Unmanaged is complaining that "Type string is not assignable to type Exclude<ExtractPropertyNamesOfTypeExcludingNullability<T, Counter>, keyof AnyRealmObject | ExtractPropertyNamesOfType<...> | ExtractPropertyNamesOfTypeExcludingNullability<...>> | ... 5 more ... | Exclude<...>"": community...living-with:realm-js:nested-unmanaged-types

Since we skipLibCheck in our ts-config, this doesn't overly affect our usage, but it's not in a good publishable state for the project itself.

@liamjones liamjones force-pushed the nested-unmanaged-types branch from 1a45d08 to 753ffd4 Compare December 11, 2024 11:12
@liamjones
Copy link
Author

@kraenhansen As I'm currently stuck with getting the PR into a mergeable state I'm patching the types in a fork for our purposes (main...living-with:realm-js:nested-unmanaged-types) and then transferring the changes into our project using patch-package.

My fork's commits are on top of the community/v20.1.0 tag and I'm using npm test --workspace @realm/node-tests to check things are good + produce the d.ts files.

One niggle I'm hitting, the build still seems to produce device sync info in the types but the realm types inside my project's node modules from v20.1.0 don't seem to contain that anymore so I'm getting more difference between the generated types than I expect (fork on the left, realm 20.1.0 inside a project's node_modules on the right):

image

How do I generate the type files without the device sync entries?

@kraenhansen
Copy link
Member

kraenhansen commented Dec 17, 2024

How do I generate the type files without the device sync entries?

These types were removed from the community branch and released as v20 as part of the deprecation of device sync.

@liamjones
Copy link
Author

Oh, I'm an idiot, I thought I branched from community but I branched from main. 🫣 Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type issues with Unmanaged<T, RequiredProperties>
2 participants