Implement `User.state` #5712

elle-j · 2023-04-05T20:36:39Z

What, How & Why?

Added missing implementation for User.state and updated the UserState enum to align with the v11 implementation.

Related v11 issue:

Incorrect Enum Value for UserState? #5203
- Implementation differs from type definitions.

This closes #5686.

☑️ ToDos

📝 Changelog entry
🚦 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

CHANGELOG.md

elle-j · 2023-04-05T20:38:23Z

integration-tests/tests/src/tests/sync/user.ts

@@ -323,6 +324,26 @@ describe.skipIf(environment.missingServer, "User", () => {
        expect(user2).to.be.undefined;
        expect(didFail).to.be.true;
      });
+
+      describe("state", () => {
+        it("can fetch state when logged in with email password", async function (this: AppContext & RealmContext) {


Note: Following it description convention of this file.

takameyer · 2023-04-06T11:49:04Z

LGTM

CHANGELOG.md

kraenhansen · 2023-04-13T14:18:18Z

packages/realm/src/app-services/User.ts

 export enum UserState {
  /** Authenticated and available to communicate with services. */
-  Active = "active",
+  LoggedIn = "LoggedIn",
  /** Logged out, but ready to be logged in. */
-  LoggedOut = "logged-out",
+  LoggedOut = "LoggedOut",
  /** Removed from the app entirely. */
-  Removed = "removed",
+  Removed = "Removed",
 }


I don't think we should make this change.

I like that the string value of these are lowercase, if users compare with strings instead of enums (non TS and potential future users, if we choose to adopt string unions over string enums as we've had conversations about elsewhere). I believe this is the case for other string enums across the code-base too.

While I understand this was the implementation on v11, it wasn't what we declared: https://github.com/realm/realm-js/blob/v11/types/app.d.ts#L538-L545

Yeah this was the most uncertain change, whether to:

(A) Break the declared types (in order to not break the implementation of v11), or

(B) Break the implementation (in order to not break the declared types, and fix the implementation bug)

Since both do break something, it's less clear which we should opt for to make a better transition from v11 - v12.

Fixing the implementation bug also seems reasonable, but if people have been using these types, they were probably depending on the (bug) implementation rather than the declarations. But I do agree on keeping the enum values lowercase, just wasn't sure if that should be a fix for v12 or v13.

What are your thoughts on (A) and (B) and how do you reason about what to break specifically for adopting v12?

Worth noting though, is the JS docs is aligned with the implementation: https://www.mongodb.com/docs/realm-sdks/js/latest/Realm.User.html#state
I'd personally vote for B as it'll make the implementation consistent with other enums and the types, which also declare our API (admittedly conflicting with both implementation and docs 😞). In a situation like this, I'd generally lean towards the declared API, but since that's not even consistent across types and docs, we'll probably have to make a tough decision.

Ty for the link! Okay yeah definitely a tougher choice but interesting to get perspective on how to navigate this as well. Very helpful input 🙂

@takameyer and @kneth, what are your thoughts on this?

Now that i'm looking into it, i'm very concerned about how much is mismatched regarding this state. If one does a search in our project for LoggedOut and then a search for logged-out then it is visible that we are not aligned throughout the project.

With that in mind, it is imperative that on both v11 and v12, we align with the C++ implementation (LoggedOut). This could also mean a breaking change release for realm-web in order to stay aligned throughout all packages.

Then I would say, we create a ticket for v13 to do a breaking change and set these to lower cased (logged-out).

My 2-cents

@takameyer to ensure I understand you, which of the two alternatives suggested by LJ are you voting for? 🙂 Are you suggesting A + following up with a ticket to break this back to lower-case with v13?

This could also mean a breaking change release for realm-web

realm-web is using the lower-cased variant: https://github.com/realm/realm-js/blob/main/packages/realm-web/src/User.ts#L57-L64

A + follow up breaking change

Although we might need to break realm-web to make it work, if the types are being shared

I tend to lean slightly more toward not changing the implementation as well (i.e. using LoggedOut etc.) for v12. But I do see both points.

To make it less annoying for users, we could actually also keep the UserState.Active but have the value be LoggedIn. This would still break the declared type, but writing UserState.Active would work. (Then mark it deprecated and remove it in v13):

v12:

export enum UserState { /** @deprecated Will be removed in v13. Please use {@link LoggedIn} */ Active = "LoggedIn", LoggedIn = "LoggedIn", LoggedOut = "LoggedOut", Removed = "Removed", }

v13:

export enum UserState { LoggedIn = "logged-in", LoggedOut = "logged-out", Removed = "removed", }

Since no apps have ever been able to use UserState.Active I think it would be better to leave its value as "Active" to avoid introducing a subtle bug in an end-users app, but just never return that from the user.state.

Agreed 🙂

* main: Update core to v13.9.1 (#5739) Implement `User.state` (#5712) Rename `local-mongodb` to enable app replacements (#5734) Update return type of 'assert.never'. (#5718) Run tests after builds are done, regardless of they failed or succeeded (#5729)

elle-j requested review from takameyer, kneth and kraenhansen April 5, 2023 20:36

cla-bot bot added the cla: yes label Apr 5, 2023

github-actions bot assigned elle-j Apr 5, 2023

elle-j commented Apr 5, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

elle-j commented Apr 5, 2023

View reviewed changes

takameyer approved these changes Apr 6, 2023

View reviewed changes

kneth approved these changes Apr 12, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kraenhansen reviewed Apr 13, 2023

View reviewed changes

elle-j added 4 commits April 17, 2023 14:15

Implement 'User.state'.

dc16822

Add CHANGELOG entry.

c7dc70e

Re-add 'UserState.Active' as deprecated.

817ff6b

Update CHANGELOG entry.

ebc0b64

elle-j force-pushed the lj/user-state branch from ed7b264 to ebc0b64 Compare April 17, 2023 12:25

Update CHANGELOG entry.

163f307

elle-j merged commit 301b6ce into main Apr 17, 2023

elle-j deleted the lj/user-state branch April 17, 2023 14:20

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `User.state` #5712

Implement `User.state` #5712

elle-j commented Apr 5, 2023 •

edited

Loading

elle-j Apr 5, 2023

takameyer commented Apr 6, 2023

kraenhansen Apr 13, 2023 •

edited

Loading

elle-j Apr 13, 2023 •

edited

Loading

kraenhansen Apr 14, 2023 •

edited

Loading

elle-j Apr 14, 2023

takameyer Apr 14, 2023

kraenhansen Apr 14, 2023

takameyer Apr 14, 2023

elle-j Apr 14, 2023

kraenhansen Apr 17, 2023

elle-j Apr 17, 2023

Implement User.state #5712

Implement User.state #5712

Conversation

elle-j commented Apr 5, 2023 • edited Loading

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

takameyer commented Apr 6, 2023

kraenhansen Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

kraenhansen Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `User.state` #5712

Implement `User.state` #5712

elle-j commented Apr 5, 2023 •

edited

Loading

kraenhansen Apr 13, 2023 •

edited

Loading

elle-j Apr 13, 2023 •

edited

Loading

kraenhansen Apr 14, 2023 •

edited

Loading