Store `state.accounts[].realm` as a WHATWG URL object. by chrisbobbe · Pull Request #4235 · zulip/zulip-mobile

chrisbobbe · 2020-08-21T06:27:20Z

Prompted recently by a performance concern in #4230 (see discussion), this is also something we've been thinking of in the background as part of #4146.

It was refreshing to see a recent, relevant issue in Jest get resolved quite quickly, and (maybe even more refreshing!) to be able to just bump up a few minor versions to get the fix. It was so easy because of the recent jump in a few major Jest versions we did recently. 🎉

chrisbobbe · 2020-09-16T19:19:10Z

Just resolved a conflict. This remains the blocker for #4230.

gnprice

Thanks @chrisbobbe ! I've now read through the first half or so of this branch:
3f9892d jest: Prepare for eslint-plugin-jest upgrade.
1c2d730 deps: Add typescript as dev dependency.
affc920 jest: Update all Jest-related dependencies to their latest.
10622c6 render test: Use representative realm value for messageTypingAsHtml.
ceedce9 url tests: Use representative realm value.
ee2764b url: Make getFullUrl use URL constructor.
20427e7 url [nfc]: Inline all uses of getFullUrl and remove it.
26272d8 webAuth: Better compare web auth callback's realm with current realm.
0e8820e store: Replace/revive URL instances.

and have just the comments below.

gnprice · 2020-09-17T05:20:01Z

    beforeEach(() => {
+      // This Jest lint rule is slightly concerning; its doc [1]
+      // claims that `expect` statements won't run if they're outside
+      // a `test` or `it` block. Empirically, here, that isn't true --
+      // changing the below assertion to its negation causes quite
+      // visible failure output from Jest.
+      //
+      // [1] https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md
+      // eslint-disable-next-line jest/no-standalone-expect
      expect(isSentryActive()).toBeFalse();
    });


How about just inlining this beforeEach and afterEach into each of the two test blocks? That seems like it'll be simpler than this comment. 🙂

From the docs:
https://jestjs.io/docs/en/api#beforeeachfn-timeout
it seems like this isn't in line with the intended usage of these methods anyway -- they're meant to be for setting up and cleaning up state that the tests need to use.

gnprice · 2020-09-17T05:22:50Z

    test('breadcrumbs', () => {
-      Sentry.addBreadcrumb({
-        message: 'test message',
-        level: Sentry.Severity.Debug,
-      });
+      expect(() => {
+        Sentry.addBreadcrumb({
+          message: 'test message',
+          level: Sentry.Severity.Debug,
+        });
+      }).not.toThrow();
    });


Then also we can skip this diff hunk. I'm pretty sure the intent of this test isn't "call this method and make sure it doesn't throw", but "call this method and check that it doesn't cause Sentry to become active". Evidently that's not how you read it, though 🙂 , so making that explicit is another reason it'd be good to simply write those expect calls inline within the test.

gnprice · 2020-09-17T05:29:32Z

        };
        fetch.mockResponseSuccess(JSON.stringify(response));

-        expect.assertions(2);


Cool -- this expect(…).rejects API sure is nicer than the old one!

gnprice · 2020-09-17T05:46:42Z

+      //
+      // This Jest lint rule is slightly concerning; its doc [1]
+      // claims that `expect` statements won't run if they're outside
+      // a `test` or `it` block. Empirically, here, that isn't true --
+      // changing the below assertion to its negation causes quite
+      // visible failure output from Jest.
+      //
+      // [1] https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md
+      // eslint-disable-next-line jest/no-standalone-expect
      expect(heartbeat.isActive()).toBeFalse();


Hmm. This one is a lot like the sentry-test.js case -- but the checks are a lot more code (vs. 1 simple line), so inlining them isn't so appealing.

If that were the only thing, then a good solution could be to put these all in a helper function and call that at the end of each test. But: looking closer at what's being checked here -- particularly the check I've highlighted -- it seems like the point of it isn't really so much to be part of the test, as it was in sentry-test.js, but to protect the tests from each other by ensuring they don't leave active heartbeats lying around. So from that perspective, it's most appropriate to have them here in afterEach rather than invoked explicitly from each test.

So I investigated this rule a bit. Not only does its description empirically seem wrong about calling expect inside afterEach -- but I also tried putting expect(2 + 2).toBe(5) (a) outside the afterEach call, at the toplevel of the describe block, and (b) at the toplevel of the whole module, and in both cases Jest gave an appropriate error. So I think the jest/no-standalone-expect rule is just mistaken, and we should probably just disable it globally.

(I did that testing at the tip of this branch, with its Jest upgrade. So there's a small possibility that this is behavior which has changed in Jest recently, and the rule used to make more sense than it does now.)

A few occurrences of these rules will newly be flagged with the upcoming `eslint-plugin-jest` upgrade: jest/expect-expect: https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md jest/no-try-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md jest/no-standalone-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. It did, however, lead to an improvement in `sentry-test`, where we move some `expect`s from `beforeEach` and `afterEach` into the tests themselves, since logically they're part of the tests themselves [1]. [1] zulip#4235 (comment)

chrisbobbe · 2020-09-17T19:35:09Z

Thanks for the review! I've pushed changes to address your comments.

gnprice

Updates look good! Just the small comments below.

I still need to read the remaining commits in the branch; but for the commits mentioned above at #4235 (review) , feel free to merge after these comments. Or a prefix of them, if you'd rather e.g. save "store: Replace/revive URL instances." to go along with the subsequent commits that use it.

gnprice · 2020-09-18T22:10:36Z

+  #
+  # [1] https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md
+  # [2] https://github.com/zulip/zulip-mobile/pull/4235#discussion_r489984717
+  jest/no-standalone-expect: off


Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. It did, however, lead to an improvement in `sentry-test`, where we move some `expect`s from `beforeEach` and `afterEach` into the tests themselves, since logically they're part of the tests themselves [1].

I do appreciate that these lint rules highlighted this improvement for us. But I think the credit can be assigned just as well to jest/expect-expect, which we're keeping 🙂 . The two tests that were improved were tests that had no expect calls of their own.

In fact I think it's not a coincidence that the tests that were improved were tests that would have been flagged by expect-expect alone, while the tests that wouldn't have been improved were tests that that rule thought was fine and only no-standalone-expect complained about. The fact that those Sentry tests had no expect of their own is closely tied to the fact that they were relying on afterEach to do the actual testing; and the fact that the heartbeat tests were more self-contained tests and their afterEach was more about protecting the tests from each other is closely tied to the fact that those tests had their own expect calls where they did their job as tests.

Makes sense!

gnprice · 2020-09-18T22:15:47Z

+  const url = tryParseUrl(callbackUrl);
+
+  if (!url) {
+    return null;
+  }


nit: for this sort of "check return value" pattern, no blank line -- the check and the line above it that produces the value are closely connected, so best to keep them visually together

A few occurrences of these rules will newly be flagged with the upcoming `eslint-plugin-jest` upgrade: jest/expect-expect: https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md jest/no-try-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md jest/no-standalone-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. See more discussion of that one on GitHub [1] [2]. [1] zulip#4235 (comment) [2] zulip#4235 (comment)

A few occurrences of these rules will newly be flagged with the upcoming `eslint-plugin-jest` upgrade: jest/expect-expect: https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/expect-expect.md jest/no-try-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md jest/no-standalone-expect https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-standalone-expect.md Adapt to the first two and disable the third, which seems unhelpful, as noted in code comments. See more discussion of that one on GitHub [1] [2]. [1] #4235 (comment) [2] #4235 (comment)

chrisbobbe · 2020-09-18T23:38:36Z

I still need to read the remaining commits in the branch; but for the commits mentioned above at #4235 (review) , feel free to merge after these comments. Or a prefix of them, if you'd rather e.g. save "store: Replace/revive URL instances." to go along with the subsequent commits that use it.

OK, done, thanks!

chrisbobbe · 2020-09-21T22:30:51Z

OK, just resolved some conflicts.

I also found a wrinkle and pushed a fix, but I feel slightly uneasy about it; maybe there's a better way? It's implemented in

8b96642 webview: Prepare to "revive" realm in handleInitialLoad.

and a small tweak in that same file (js.js) in

8c0cc3f accounts: Make Auth.realm a URL object, including in state.accounts.

gnprice · 2020-09-22T00:03:01Z

I also found a wrinkle and pushed a fix, but I feel slightly uneasy about it; maybe there's a better way? It's implemented [...] in

8c0cc3f accounts: Make Auth.realm a URL object, including in state.accounts.

Hmm, I see!

Seems like one underlying awkward thing here is that the call site of handleInitialLoad is not type-checked. Which is because it's generated code, from this template string:

export default (scrollMessageId: number | null, auth: Auth): string => `
<script>
// …
document.addEventListener('DOMContentLoaded', function() {
  ${compiledWebviewJs}
  compiledWebviewJs.handleInitialLoad(
    ${JSON.stringify(Platform.OS)},
    ${JSON.stringify(scrollMessageId)},
    ${JSON.stringify(auth)}
  );
});
</script>
`;

... But then when thinking about ways to address that, I find another underlying awkward thing: our JSONable type does not quite mean what it says it means, which is values that can be round-tripped through JSON. In particular, it has URL as a subtype. Demo here.

So I don't have a great idea for how to arrange this so that the computer checks we haven't missed something, for when these types potentially change again in the future. Failing that, this seems like a fine way to handle the types as they currently are.

gnprice

Looks good! Small comments below.

I went through the codebase with the search rg '[rR]ealm.toString' to look for likely places to convert further toward URL objects, and only found a couple of items, mentioned below. Also rg 'new URL\(.*realm' and didn't find anything.

On another side of things, I did a pair of searches for comparisons that potentially need toString() added: rg -i 'realm.*[!=]==' and rg -i '[!=]==.*realm'. Everything looked to be covered.

gnprice · 2020-09-22T00:04:36Z

+  const auth: Auth = { ...rawAuth, realm: rawAuth.realm };
  // Since its version 5.x, the `react-native-webview` library dispatches our


nit: blank line to separate these

gnprice · 2020-09-22T00:20:58Z

    });

-    openLink(`${this.props.realm}/complete/apple/?${params}`);
+    openLink(`${this.props.realm.toString()}/complete/apple/?${params}`);


This probably wants to be a URL computation.

(That'd be a followup, another thing we can do to take advantage of having URL objects everywhere; for this branch a TODO comment would be fine.)

gnprice · 2020-09-22T00:22:53Z

-  isUrlOnRealm(url, realm) ? /^(\/#narrow|#narrow)/i.test(url.split(realm).pop()) : false;
+export const isInternalLink = (url: string, realm: URL): boolean =>
+  isUrlOnRealm(url, realm)
+    ? /^(\/#narrow|#narrow)/i.test(url.split(realm.toString()).pop())


This bit deserves a TODO comment about replacing it with URL, similar to a couple of other such comments added in this branch.

In an upcoming commit, we'll change the `Auth` type to have a URL object for the realm. We call `JSON.stringify` on the arguments passed to `handleInitialLoad`. For the `realm` property, this means it'll be a string URL, not a URL object. Here, we pick it out in preparation for "reviving" it back into its URL form in the body of `handleInitialLoad`.

Like we did in bfe7949, for ZulipVersion instances. We don't store any of these in Redux yet, but we will soon.

Changing the `Auth` type implicitly changes the `Account` and `Identity` types, too. Done with an effort to minimize unnecessary follow-on changes in this commit. Here, we don't propagate the realm in its URL form as far toward the eventual consumers of the realm as we'll eventually want to; we'll do that in upcoming commits. Wherever two realm values are compared for equality with `===`, we make sure they are both stringified first. Flow didn't warn about these cases, so we caught them in test failures and by scanning through a full-project search for the term 'realm'.

As we'll do in some upcoming commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.

As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.

As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. `getAvatarUrl` itself will likely disappear soon, when we make the shell crunchier for handling `.avatar_url` on users, cross-realm bots, and messages. But the changes we make to its callers, here, will continue to be beneficial after that.

As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. Also rename `this.state.realm` to `this.state.realmInputValue`, to be more descriptive.

…mUrl`. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.

As in the preceding handful of commits, stringify the realm exactly where we need it as a string. These should be the last sites in which it's not totally clear that the realm is well-formed data.

chrisbobbe · 2020-09-22T00:54:42Z

Thanks for the review! Glad to catch those things. 🙂 I've just pushed my changes.

chrisbobbe · 2020-09-22T01:42:14Z

In fact, this might not need another review; your suggestions are small and were easily addressed. I'll merge this in the next few minutes unless you say otherwise. 🙂

chrisbobbe requested a review from gnprice August 21, 2020 06:27

chrisbobbe force-pushed the pr-realm-url-object branch 2 times, most recently from 386383a to c00b9e6 Compare August 24, 2020 19:07

chrisbobbe mentioned this pull request Aug 24, 2020

Crunchify shell for avatar URLs, handle user_avatar_url_field_optional client capability #4230

Merged

chrisbobbe force-pushed the pr-realm-url-object branch from c00b9e6 to 7f4aa27 Compare September 16, 2020 19:18

gnprice reviewed Sep 17, 2020

View reviewed changes

chrisbobbe force-pushed the pr-realm-url-object branch from 7f4aa27 to f0cbe6b Compare September 17, 2020 19:34

gnprice reviewed Sep 18, 2020

View reviewed changes

chrisbobbe force-pushed the pr-realm-url-object branch from f0cbe6b to c2d3026 Compare September 18, 2020 23:27

chrisbobbe force-pushed the pr-realm-url-object branch 3 times, most recently from ef64362 to 24c51e5 Compare September 21, 2020 22:30

gnprice reviewed Sep 22, 2020

View reviewed changes

chrisbobbe added 9 commits September 21, 2020 17:43

store: Replace/revive URL instances.

565e23e

Like we did in bfe7949, for ZulipVersion instances. We don't store any of these in Redux yet, but we will soon.

webAuth: Pipe Account[0].realm as URL down to authFromCallbackUrl.

a3a8317

As we'll do in some upcoming commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.

react: Pipe Account[].realm as URL to some React-component consumers.

e5972a2

As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.

url, internalLinks: Pipe realm as URL to isUrlOnRealm, `getPathsFro…

458dee2

…mUrl`. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data.

redux: In REALM_ADD and LOGIN_SUCCESS, make realm be a URL object.

ed1be64

As in the preceding handful of commits, stringify the realm exactly where we need it as a string. These should be the last sites in which it's not totally clear that the realm is well-formed data.

url handling [nfc]: Add a few TODOs for clearer, better code.

05e24ee

chrisbobbe force-pushed the pr-realm-url-object branch from 24c51e5 to 05e24ee Compare September 22, 2020 00:51

chrisbobbe merged commit 05e24ee into zulip:master Sep 22, 2020

This was referenced Oct 28, 2020

Opening a notification from another account causes crash #4290

Closed

Use real URL parsing for links found in messages, particularly to narrows #4292

Open

notifications: normalize case of realm_uri and account.realm #3671

Closed

chrisbobbe deleted the pr-realm-url-object branch November 5, 2021 00:03

		const auth: Auth = { ...rawAuth, realm: rawAuth.realm };
		// Since its version 5.x, the `react-native-webview` library dispatches our

Uh oh!

Conversation

chrisbobbe commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrisbobbe commented Sep 17, 2020

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 18, 2020

Uh oh!

chrisbobbe commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 22, 2020

Uh oh!

chrisbobbe commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe commented Aug 21, 2020 •

edited

Loading

chrisbobbe commented Sep 16, 2020 •

edited

Loading

chrisbobbe commented Sep 21, 2020 •

edited

Loading

gnprice commented Sep 22, 2020 •

edited

Loading