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

Validate configuration #3772

Merged
merged 10 commits into from
Jun 10, 2021
Merged

Validate configuration #3772

merged 10 commits into from
Jun 10, 2021

Conversation

kneth
Copy link
Contributor

@kneth kneth commented Jun 1, 2021

What, How & Why?

This closes #3771

☑️ ToDos

  • 📝 Changelog entry
  • [ ] 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • [ ] 📝 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
  • [ ] Chrome debug API is updated if API is available on React Native

@kneth kneth requested a review from fronck June 1, 2021 15:18
@kneth kneth self-assigned this Jun 1, 2021
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Is disableFormatUpgrade an option compatible with sync? I would imagine that this would prevent Sync from writing to the file. Additionally, we should express these incompatibilities in the typescript definitions so that people who are using an editor that respects ts definitions will get early feedback if they try to construct an invalid config.

@kneth kneth force-pushed the kneth/mutual-exclusive-inmem-sync branch from dd7ebe9 to d60c373 Compare June 8, 2021 13:32
@kneth kneth requested review from nirinchev and kraenhansen June 8, 2021 13:32
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

Just a formatting issue that should be addressed.

I am not completely satisfied with how the typings resolve for configuration, but I have no alternative solution at the moment. If the developer explicitly uses ConfigurationWithSync, then the TS errors look nice, but if they use the Configuration union, the TS errors are quite vague. If I come up with a better solution later, I will make a separate PR.

Otherwise, LGTM

types/index.d.ts Outdated Show resolved Hide resolved
kneth and others added 2 commits June 10, 2021 09:39
@kneth kneth requested a review from takameyer June 10, 2021 07:41
@kneth kneth merged commit 81f0e1b into master Jun 10, 2021
@kneth kneth deleted the kneth/mutual-exclusive-inmem-sync branch June 10, 2021 08:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow in-memory for sync Realms
4 participants