Make new mentioned/starred messages appear in mentions/starred narrows

[chrisbobbe]

Fixes #3592.

I ran into some Babel issues with Jest and haven't been able to run the tests yet; could you please take a quick look at the logs (attached) to see if this is a known issue? Since I'm assuming everything works for people with development environments that have been set up for a while, and the error was thrown from code I didn't touch, there may have been a breaking change in a minor version of Babel or Jest that was triggered by the carat in package.json allowing silent minor version upgrades (e.g., "jest": "^24.8.0"), but that's just a guess until I can dig deeper or learn that it's a known issue.

Issue 3592 Test Output (babel issue?).txt

I'm proposing a change to the eventMiddleware logic (first commit) that would ensure that flags is always set on action.message.flags, at least as an empty array, if event.flags does not exist, to help with the "crunchy shell" pattern: https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md.

[chrisbobbe]

Also, I used a rebase to reorder the commits so that the eventMiddleware commit (ensuring the existence of message.flags; ca5f351) was done first, since technically it is required to avoid an error that would have been introduced in the 2fa1684 commit, since that commit has a call to .includes on message.flags without first checking that message.flags exists. You can see this reorder in my fork: https://github.com/chrisbobbe/zulip-mobile/commits/issue-3592 but I'm puzzled why my original order (the incorrect order) is showing up here.

[chrisbobbe]

Oh interesting, I can see the test results from Travis; it says, "TypeError: Cannot add property flags, object is not extensible".

If event.message not extensible now, I wonder why it wasn't treated as extensible before my commits; it was still assigning .flags to event.message where it was not defined before, right? I'm assuming this test passed before my commits; once I get Jest working locally, I can dig deeper into this. At a glance right now, I've traced event back to the JSON from the fetch() call I and haven't found a call to Object.freeze() or Object.preventExtensions().

Summary of all failing tests
 FAIL  src/events/__tests__/eventMiddleware-test.js
  ● eventMiddleware › if `event.flags` does not exist, set message.flags to an empty array
    TypeError: Cannot add property flags, object is not extensible
      18 |       if (!event.message.flags) {
      19 |         // $FlowFixMe Message is readonly to serve our use of it in Redux.
    > 20 |         event.message.flags = event.flags || NULL_ARRAY;
         |                               ^
      21 |         delete event.flags;
      22 |       }
      23 | 
      at _default (src/events/eventMiddleware.js:20:31)
      at Object.<anonymous> (src/events/__tests__/eventMiddleware-test.js:26:34)

[chrisbobbe]

Oops -- I didn't see your comment from yesterday on the issue:

Over on the linked thread we're still sorting out exactly how to handle wildcard_mentioned. So for this issue, let's not worry about it.

until now, having already implemented an opinion about 'wildcard_mentioned' (i.e., that it's a separate flag that needs to be checked everywhere we check 'mentioned'). Sorry about that; I'll make changes of course, as necessary, just let me know.

[gnprice]

Thanks @chrisbobbe !

Oops -- I didn't see your comment from yesterday [about wildcard_mentioned]

No problem. This looks like a good simple-to-implement way of handling it; if we end up combining the flags earlier in the pipeline, we can adjust this code then.

---

Oh interesting, I can see the test results from Travis; it says, "TypeError: Cannot add property flags, object is not extensible".

Ah, so: look closely at the top and bottom of the failure text you quoted. This is happening when running eventMiddleware-test.js... and specifically at this line:

      at Object.<anonymous> (src/events/__tests__/eventMiddleware-test.js:26:34)

That's the eventMiddleware call in the "if event.flags does not exist" test case.

So I think it's caused by the deepFreeze in that test case. The bulk of our tests use deepFreeze on their fixture data, in order to catch unintended mutations. In this case the mutation is intended, so the simple fix is to remove the deepFreeze.

(Whether using mutation here is a good idea is another question. Most likely it isn't and we'd be better off keeping things unmutated and making a new object here, just as we do in the bulk of the app. But that's probably a larger refactor.)

---

The bigger thing the CI run finds is actually a series of Flow errors. These are all of the form:

Cannot call message.flags.includes because property includes is missing in undefined [1].

That's because the Message type still allows flags to be absent, which is because it's:

   *  * Absent in the Redux `state.messages`; we move the information to a
   *    separate subtree `state.flags`.

You can run our test suite with tools/test, or just Flow with tools/test flow (or yarn flow). More info here. That will be helpful for iterating to get to a tests-clean state.

Specific comments coming next.

[gnprice]

Also, I used a rebase to reorder the commits so that the eventMiddleware commit (ensuring the existence of message.flags; ca5f351) was done first, since technically it is required to avoid an error that would have been introduced in the 2fa1684 commit, since that commit has a call to .includes on message.flags without first checking that message.flags exists.

Perfect, thanks!

You can see this reorder in my fork: https://github.com/chrisbobbe/zulip-mobile/commits/issue-3592 but I'm puzzled why my original order (the incorrect order) is showing up here.

Yeah, this is unfortunately a longtime bug in GitHub: they'll show commits inappropriately ordered by author date, making them out of order compared to the actual Git history.

(My guess has always been that someone decided topo-sorting was too hard and/or complicated, and wasn't fully aware of Git committer dates -- sorting by committer date would be implemented almost exactly the same as by author date, and fix the problem in almost all cases. But who knows.)

When reviewing a PR, I fetch it locally and mainly read it with git log --stat -p and git log --oneline (or actually my aliases git usp and git uv), rather than the GitHub web UI. So I usually don't actually even notice the order GitHub displays the commits in. This issue is one of the reasons for that approach.

[gnprice]

ca5f351 eventMiddleware: Set event.message.flags to [] even if absent on event.

I like this change! It makes the behavior simpler to describe, and also brings it in line with what that comment on Message.flags said it already was.

Test failure needs to be fixed; discussed above.

This step isn't enough to make it possible for the rest of the app to become a "soft center" with respect to this property's presence, because in the Redux state we leave it off and maintain the flags data separately. But it's still a move in the right direction.

2fa1684 isMessageInNarrow: Fix check for starred or mentioned messages.

This looks good, except the Flow error. I believe that at each call site it's actually the case that this Message has a flags property... but because we use one Message type for a few different purposes, Flow doesn't see that.

I think the best immediate fix is to just include the presence checks that the type says are necessary.

Hmm, or for a more principled alternative: assert the property is there (i.e. throw an error if not.) That way if we do introduce a bug where the property is missing here because the flags are located elsewhere, we'll almost certainly notice in dev, rather than silently behave as if all flags are false. (But if that gets cumbersome, no need; as the next commit shows, the status quo is already that we just do the presence checks and silently assume that if there were flags set they'd be present here.)

3e53bf7 Fix 2 checks of 'mentioned' flag without checking 'wildcard_mentioned'

Thanks for this fix! I agree this is the right behavior in both of these spots.

This version causes Flow errors much like the parent, for the same reason.

4638c8c narrowsReducer/eventUpdateMessageFlags: Handle mentioned/wildcard_mentioned

This looks right!

I'd like to avoid duplicating the code in this block, though. How about factoring it out into a function so that we can say something like

  if (flag === 'starred') {
    return updateFlagNarrow(state, STARRED_NARROW_STR, operation, messages);
  }
  return state;

Then to handle mentions is a short else if (['mentioned', 'wildcard_mentioned'].includes(flag)) block.

[chrisbobbe]

Oh OK, yes, I can see now that the test results showing the flow issues and the "object is not extensible" message do indeed show up when testing in my local development environment. I saw the 20 occurrences of "SyntaxError: Unexpected token import" (shown in the .txt doc in my first post here) and assumed that there was some Babel mis-configuration interfering with all tests, without looking closer to see that some tests did run properly.

It looks like this error is thrown in all 20 cases by this line in src/compose/ComposeMenu.js, a file not touched by my commits:

const DocumentPicker = (await import('react-native-document-picker')).default;

and it appears that that import has caused issues before; it's preceded by this comment:

    // Defer import to here, to avoid an obnoxious import-time warning
    // from this library when in the test environment.

Deferring the import doesn't seem to have helped much, or else it fixed the original warning and caused this new error; could it be that there was an additional workaround outside source control, like (somehow?) some more Babel configuration, that most developers have, but I don't, since I just recently cloned the project?

[chrisbobbe]

Hmm, or for a more principled alternative: assert the property is there (i.e. throw an error if not.) That way if we do introduce a bug where the property is missing here because the flags are located elsewhere, we'll almost certainly notice in dev, rather than silently behave as if all flags are false. (But if that gets cumbersome, no need; as the next commit shows, the status quo is already that we just do the presence checks and silently assume that if there were flags set they'd be present here.)

Hmm, so I've tried a few different things for this. Currently, it looks like this:

    starred: () => message.flags.includes('starred'),
    mentioned: () => message.flags.includes('mentioned') || message.flags.includes('wildcard_mentioned'),

All three calls to .includes() result in Flow errors because there is no check that .flags is defined, as you've said.

For the least amount of repeating myself, I tried adding a single assertion above:

  if (!message.flags) {
    throw new Error('`message.flags` should be defined.')
  }
  
  return caseNarrow(narrow, {
    home: () => true,
    stream: name => name === message.display_recipient,
    topic: (streamName, topic) =>
      streamName === message.display_recipient && topic === message.subject,
    pm: email => matchRecipients([email]),
    groupPm: matchRecipients,
    starred: () => message.flags.includes('starred'),
    mentioned: () => message.flags.includes('mentioned') || message.flags.includes('wildcard_mentioned'),
    allPrivate: () => message.type === 'private',
    search: () => false,
  });

All three Flow errors remained. Despite the assertion, apparently Flow can't be reasonably sure that message.flags will still be defined in the three sites where .includes() is called -- my current level of understanding is that these sites are just "too far away" from the assertion, which I guess is defensible on Flow's part, if a little annoying.

Next, I tried a version that does include repeating myself, but at least is relatively compact, since it uses a ternary instead of if/else blocks.

    starred: () => message.flags
      ? message.flags.includes('starred')
      : throw new Error('`message.flags` should be defined.'),
    mentioned: () => message.flags
      ? (message.flags.includes('mentioned') || message.flags.includes('wildcard_mentioned'))
      : throw new Error('`message.flags` should be defined.'),

This didn't work; many more errors were triggered (Flow errors with ternary attempt.txt). My first guess is that it isn't acceptable for starred or mentioned to return the type of whatever throw new Error('...') evaluates to (which doesn't 100% make sense as throw ... is a statement, not an expression). Anyway, caseNarrow expects all the functions on the object in the second argument to return the same type.

Finally, I tried fully fledged if/else blocks:

    starred: () => {
      if (message.flags) {
        return message.flags.includes('starred');
      } else {
        throw new Error('`message.flags` should be defined.');
      }
    },
    mentioned: () => {
      if (message.flags) {
        return message.flags.includes('mentioned') || message.flags.includes('wildcard_mentioned');
      } else {
        throw new Error('`message.flags` should be defined.');
      }
    },

That did fix it for the first two sites of .includes() -- I guess it was satisfied that throw new Error('...') isn't returned, as in the previous example, it's just executed -- but apparently the third site, message.flags.includes('wildcard_mentioned'), is too far away from the assertion for Flow to be informed by it, and it alone caused the error (below). Is there a guiding principle that explains why it would work for the first two sites of .includes() but not this one, other than my vague "too far away" explanation?

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈src/utils/narrow.js:233:69

Cannot call message.flags.includes because property includes is missing in undefined [1].

     src/utils/narrow.js
     230│     },
     231│     mentioned: () => {
     232│       if (message.flags) {
     233│         return message.flags.includes('mentioned') || **message.flags.includes('wildcard_mentioned')**;
                    // (** indicates a colored highlight in the logs **)
     234│       } else {
     235│         throw new Error('`message.flags` should be defined.');
     236│       }

So maybe we're at the point of "too cumbersome," unless you see something else I could try.

[gnprice]

I saw the 20 occurrences of "SyntaxError: Unexpected token import" (shown in the .txt doc in my first post here) and assumed that there was some Babel mis-configuration interfering with all tests, without looking closer to see that some tests did run properly.

Ah, apologies, I looked right past that yesterday.

I believe this means you're using Node 8, and the fix is to use Node 10 (the current LTS). The incompatibility with Node 8 was introduced last week in 338036e; it actually broke things in CI until 091bdc3 a few commits later.

The error is pretty unhelpful. Because the incompatibility is only a week old, I think you may be the first person to have gotten bitten by it. We should make that easier to debug; I might try putting a Node version check right in tools/test, or failing that we'll add a troubleshooting entry in build-run.md .

I'd appreciate seeing what node --version says for you, to confirm that that's the issue. (And then I recommend going ahead and upgrading Node.)

[gnprice]

Deferring the import doesn't seem to have helped much, or else it fixed the original warning and caused this new error; could it be that there was an additional workaround outside source control, like (somehow?) some more Babel configuration, that most developers have, but I don't, since I just recently cloned the project?

And to answer these questions directly (though you might be able to deduce some of these answers from the diagnosis above):

• Yep, deferring the import fixed the original warning. That happened originally in b20dbcf .
• Then in 338036e we adopted fancier syntax for deferring the import... and that caused this new error.
• The known way to avoid the error is to be using Node 10 rather than Node 8. This is in our setup instructions but easy to miss, especially if you already have Node lying around.
• I'm actually puzzled why the Node version matters here, because I'd have thought it's Babel's job to compile the fancy syntax out. As that commit message on 338036e says, our Babel config appears to have the plugin enabled that would compile this out. I haven't dug into the discrepancy; when CI failed my first question was what was different between CI and my machine, and when I saw we were using an old Node version in CI and that upgrading that fixed the issue, I left it at that. There probably is something wrong in our Babel config (perhaps only for Jest?), but I expect that part doesn't differ between your machine and others'.

[gnprice]

there may have been a breaking change in a minor version of Babel or Jest that was triggered by the carat in package.json allowing silent minor version upgrades (e.g., "jest": "^24.8.0"), but that's just a guess until I can dig deeper or learn that it's a known issue.

Ah, and this at least shouldn't be a problem -- avoiding that kind of discrepancy is Yarn's job, with yarn.lock. 🙂 In particular these lines:

jest@^24.8.0:
  version "24.8.0"
  resolved "https://registry.yarnpkg.com/jest/-/jest-24.8.0.tgz#d5dff1984d0d1002196e9b7f12f75af1b2809081"
  integrity sha512-o0HM90RKFRNWmAWvlyV8i5jGZ97pFwkeVoGvPW1EtLTgJc2+jcuqcbbqcSZLE/3f2S5pt0y2ZBETuhpWNl1Reg==

should mean that yarn install (or plain yarn) gets exactly version 24.8.0 (and with exactly that SHA-512 hash) for that "jest": "^24.8.0".

Upgrades are done explicitly with e.g. yarn upgrade, and are reflected in yarn.lock so that everyone else gets the same new versions once they rerun yarn.

[gnprice]

All three Flow errors remained. Despite the assertion, apparently Flow can't be reasonably sure that message.flags will still be defined in the three sites where .includes() is called -- my current level of understanding is that these sites are just "too far away" from the assertion, which I guess is defensible on Flow's part, if a little annoying.

Yeah. I think specifically the issue is that message is an object, it's mutable, and there's no way for Flow to know that it doesn't get mutated sometime in caseNarrow or before one of those callbacks gets invoked. (Maybe it could with a sufficiently smart whole-program analysis, but for scalability it doesn't do that; all inference is local, and the only things it knows from the rest of the program are what's in the types.)

The most common fix for this kind of thing is to say something like

const { flags } = message;

and then have both the check and the use refer to flags. Then it's clear they both see the same flags, even if message is mutated in between.

In our codebase this is mostly a (small) annoyance, because for most objects the truth is they never get mutated. In codebases where mutation is routine, and especially those with a lot of concurrency, this pattern is actually a very important one for avoiding some frustrating bugs.

[gnprice]

That did fix it for the first two sites of .includes() -- I guess it was satisfied that throw new Error('...') isn't returned, as in the previous example, it's just executed -- but apparently the third site, message.flags.includes('wildcard_mentioned'), is too far away from the assertion for Flow to be informed by it, and it alone caused the error (below). Is there a guiding principle that explains why it would work for the first two sites of .includes() but not this one, other than my vague "too far away" explanation?

Ah, yeah. It's the same principle at work as in your first version:

I think specifically the issue is that message is an object, it's mutable, and there's no way for Flow to know that it doesn't get mutated sometime in caseNarrow or before one of those callbacks gets invoked.

Here, it's that once the first includes method is called, that's some arbitrary code running -- and Flow can't tell whether that code might go and mutate other things in the program, including our message object.

So the general fix to make a guard condition stay good is to make a local const binding for whatever it's inspecting. Otherwise, it'll last just until the first function call.

[chrisbobbe]

Yep, that was it, thank you! node --version gives v8.12.0, and switching to v10.10.0 fixes the issue completely.

The most common fix for this kind of thing is to say something like

const { flags } = message;
and then have both the check and the use refer to flags. Then it's clear they both see the same flags, even if message is mutated in between.

Brilliant, this worked! Yes, I was referring to the possibility of mutation with "which I guess is defensible on Flow's part, if a little annoying"; I just took a step back when I saw that an assertion is respected for code that immediately follows it, so I wasn't sure how far away was "too far away," and how that immediacy is determined.

Here, it's that once the first includes method is called, that's some arbitrary code running -- and Flow can't tell whether that code might go and mutate other things in the program, including our message object.

This sounds correct for what's going on here; some supporting examples to follow.

Otherwise, it'll last just until the first function call.

And this sounds reasonable as the policy to determine where "too far away" is -- it's easy on computation; like you said, it can't spend too many resources predicting state in runtime or it wouldn't scale.

Some tweaking confirms that you can introduce arbitrary code between the check and the call to .includes, without triggering the error, as long as it doesn't include a function call (below). (Even a simple console.log('...'); will trigger the error, which also suggests that the policy is to check for the first function call, no matter what that function is.)

      // no Flow error
      if (message.flags) {
        const asdf = 'asdf';
        const o = { a: 123, b: 456 };
        delete o.a;
        for (let i = 0; i < 10; i++) {
          let jkl = 'jkl';
        }
        return false || message.flags.includes('wildcard_mentioned')
      } else {

Surprisingly, it still doesn't trigger the error in the following case, presumably because there are still no function calls:

      // still no Flow error (oops!)
      if (message.flags) {
        delete message.flags; // <---- message.flags is gone, but still no error!
        return false || message.flags.includes('wildcard_mentioned') // <---- unsafe call!
      } else {

EDIT: Ah, I did finally trigger the error without a function call in a pretty obvious way (below), but the delete case (above) still seems like a striking omission.

      // Yes, Flow error this time
      if (message.flags) {
        message.flags = 123; <---- it now considers message.flags to be a Number
        return false || message.flags.includes('wildcard_mentioned')
      } else {

[gnprice]

Slightly upsettingly, it still doesn't trigger the error in the following case, presumably because there are still no function calls:

      // still no Flow error (oops!)
      if (message.flags) {
        delete message.flags; // <---- message.flags is gone, but still no error!
        return false || message.flags.includes('wildcard_mentioned')
      } else {

Hmm, indeed!

It looks like the Flow bug here is centered on delete. If we mutate the property to some other value rather than delete it, Flow gives an error appropriately:

const m = ({x: 1}: {| x: number | null |});
if (m.x !== null) {
  m.x = null;
  m.x.toFixed(); // error: Cannot call `m.x.toFixed` because property `toFixed` is missing in null
}

Looks like facebook/flow#6567 probably covers this, and facebook/flow#7089 seems closely related.

[chrisbobbe]

Upgrades are done explicitly with e.g. yarn upgrade, and are reflected in yarn.lock so that everyone else gets the same new versions once they rerun yarn.

Nice; I had a vague feeling the lock file could also be updated with a yarn install (or perhaps a yarn install following the initial install), but I suppose not, which is nice.

I've made some changes to address your feedback; the plus sign (+) tip at https://zulip.readthedocs.io/en/latest/git/fixing-commits.html#pushing-commits-after-tidying-them really helped me out when I thought I was stuck, with no way to avoid introducing a merge commit! :)

[gnprice]

And merged. Thanks again @chrisbobbe for the thorough fix and for the revisions!

Seeing the 'add' | 'remove' assert in your last commit, I added a small commit on top that puts a static type-level assert there as well (which isn't a substitute here for the runtime one); take a look.

[gnprice]

We should make that easier to debug; I might try putting a Node version check right in tools/test, or failing that we'll add a troubleshooting entry in build-run.md .

Did both of these! Just pushed commits 9f82dfb and a5a064f.

[chrisbobbe]

You're welcome, glad to help!

Seeing the 'add' | 'remove' assert in your last commit, I added a small commit on top that puts a static type-level assert there as well (which isn't a substitute here for the runtime one); take a look.

Noted, thanks!