realm-js icon indicating copy to clipboard operation
realm-js copied to clipboard

Error Realm initialization with sync enabled due to mismatch schema is not calling onError function

Open gfrancischini opened this issue 1 year ago • 11 comments

How frequently does the bug occur?

Always

Description

Check the following line:

https://github.com/realm/realm-js/blob/91de9446242a3dc7aaaf640925af360b5d91a7d2/packages/realm-react/src/RealmProvider.tsx#L140

It is calling the console.error directly. This way we cannot handle this error on user device.

We have configured the application to only open the realm for the first time after DownloadBeforeOpen.

Stacktrace & log output

[Error: The following changes cannot be made in additive-only schema mode:
- Property 'ObjectType.idInitialStatusIntegration' has been made required.
- Property 'ObjectType.idInitialStatusPDA' has been made required.
- Property 'ObjectType.idInitialStatusWeb' has been made required.
- Property 'ObjectType.nmCdField' has been made required.
- Property 'ObjectType.nmDisplayField' has been made required.
- Property 'ObjectType.nmIdField' has been made required.
- Property 'ObjectType.nmTable' has been made required.
If your app is running in development mode, you can delete the realm and restart the app to update your schema.]

Can you reproduce the bug?

Always

Reproduction Steps

  1. Create a sample app
  2. Set to open the realm for the first time DownloadBeforeOpen
  3. Change the schema to a breaking schema
  4. delete the local realm
  5. Open the app again and see that the error is only printed at the console

Version

[email protected]

What services are you using?

Atlas Device Sync

Are you using encryption?

No

Platform OS and version(s)

Android/iOS All

Build environment

Which debugger for React Native: ..

Cocoapods version

No response

gfrancischini avatar Apr 08 '24 11:04 gfrancischini

➤ PM Bot commented:

Jira ticket: RJS-2794

sync-by-unito[bot] avatar Apr 08 '24 11:04 sync-by-unito[bot]

@gfrancischini Thank you for reporting.

I believe that we should extend @realm/react with an option not to shallow the exceptions from Realm.open(). We can either rethrow the exception or call the onError callback (if defined).

kneth avatar Apr 09 '24 07:04 kneth

We agree an error like this should signaled differently than calling console.error:

  1. Calling onError would be difficult, because that function takes the active sync session, but at this point no sync session has been established. To call it, we would have to break the API, allowing the onError to have null passed as the sync session argument, which is not ideal. So .. since this error isn't sync specific, we would most likely want to surface it differently than though the onError.
  2. The RealmProvider could take an onError prop (admittedly, this name might be a little confusing as it conflicts with the onError passed through the sync config).
  3. We could throw when the useRealm (or any of our other hooks depending on it).
  4. Some other alternative that we haven't thought of? What's an idiomatic way to signal an error state from a React context provider? 🤔

kraenhansen avatar May 08 '24 12:05 kraenhansen

Hey any update on this issue? We are about to enter in homologation with our customer and we would like to be able to correctly log these errors to diagnose possible production problems

gfrancischini avatar May 22 '24 23:05 gfrancischini

@gfrancischini

any update on this issue?

Currently we are logging the errors to help customers to diagnose problems. To provide a better solution, we would like the customer to give us feedback on the four items in the previous comment.

kneth avatar May 23 '24 11:05 kneth

The RealmProvider could take an onError prop (admittedly, this name might be a little confusing as it conflicts with the onError passed through the sync config).

I would call it onInitializeError or onRealmInitializeError as this is something that will only happen during realm initialization?

Or maybe onMountError? but I still prefer the above

We could throw when the useRealm (or any of our other hooks depending on it). I think this is bad as it is not good to handle error on component renders

gfrancischini avatar May 23 '24 15:05 gfrancischini

I think this is bad as it is not good to handle error on component renders

I would expect developers to use an error boundary to handle these errors: Via getDerivedStateFromError on a class-based component or the resuable ErrorBoundary utility component.

kraenhansen avatar May 24 '24 17:05 kraenhansen

I feel that error boundaries should be used when something unexpectedly happens. A schema error is a handled error and in my opinion, should be handled differently. What we know/is expected should be handled different from the unknow.

But that is only my opinion really. So whatever solutions as long as we can show to our end user it is okay. What we cannot do is swallow the error as it is today as the app just crashes without giving user feedback

gfrancischini avatar May 24 '24 17:05 gfrancischini

I suppose the difference is if it's recoverable for the end-user or not 🤔 Some will be, others likely wont. I think this decision is the only thing holding back a fix for this. It's likely a simple change in code - and I'd like for us to get the API right 👍

kraenhansen avatar May 24 '24 18:05 kraenhansen

We are also in need of a way to detect model schema mismatch, so that we can tell the user what corrective action they can take, instead of the app just crashing.

Is there any planned fixed/change to allow us to detect this?

jaltin avatar Aug 29 '24 07:08 jaltin

has a decision been taken on this? at the moment this is a blocker for our project because if this happens the user will be stuck seeing a spinner and we want to show a message that a newer version of the app needs to be installed.

andrei-tofan avatar Aug 29 '24 07:08 andrei-tofan