feat(react-meteor-accounts): fixes types, deps; adds React check, loggingIn/Out sources, tests

[WilliamKelley]

Apologies for a large PR. I can split it up if needed.

Accomplished tasks:

• Added dependency on "accounts-base".
  • Otherwise, the properties used would not exist on Meteor.*.
  • *Edit: I believe this is good reason to switch to using `Accounts.` instead, but that's also trivial.**
• Corrected TS function signatures.
  • HOCs are apparently hard to type! but I looked around at some other major react packages and did what I thought was reasonable and works as demonstrated in the tests.
  • ~side note: TS wrongly excludes null from the returns types of Meteor.userId / Meteor.user, not sure why... but that's why the corresponding hooks have explicit signatures~ Edit: Solved
  • Ran the types script again; updated the README
• Added check for at least React v16.8
• Added JSDoc function summaries
• Added hooks/HOCs for Meteor.loggingIn, Meteor.loggingOut
  • ~Turns out Meteor.loggingOut isn't declared in @types/meteor (or in the online docs for that matter), but it does exist and it is useful -- hence the TS module augmentation.~ Edit: Solved
  • Added respective docs to README
• Reorganized README to document each utility in isolation.
• Added tests for all exports.
  • Since HOCs are really just hooks, I tested all edges of reactivity in hook tests, and then just significant pass-through in hoc tests.
  • Two of the HOC tests are flaky -- i have no clue why. I spent a good amount of time debugging, but everything appeared to be running and finishing in order, but the values just wouldn't update one time out of ten... Doesn't make sense to me.

Outstanding tasks:

• [ ] Clarify package name: directory is react-meteor-accounts but package.js says react-accounts.
  • other options i see: "meteor/react-meteor-accounts", react-trackers-accounts, react-accounts-trackers. I'll leave up to maintainers.
• [ ] use/withUser should implementation ([options]) for optional field projection, like Meteor.user.
  • I think this is best to leave to another PR, where useTracker or useFind is brought in with react-meteor-accounts as a dep.

Further considerations:

• I think it's best if HOCs were not contained in this package. Given that the package's minimum version guarantees React hooks, the consumer can easily compose their own HOC; this is what react-router advises if withRouter is needed by v6 consumers. This has an additional benefit: consider a consumer needs a combination of these HOCs. They must compose that combination of our HOCs. This can be unwieldly if they need all 4, and can be suboptimal to call forwardRef four times.
  • Another option is we exported a single HOC with all four sources, but then it's reactive to all four even when the user may not want that.
  • I think the solution is we simply shouldn't export HOC's, and instead adopt advice like react-router. This establishes better patterns for consumers and eases our implementation and testing burden.

[CaptainN]

I'll review this soon. There's a lot!

[WilliamKelley]

I'll review this soon. There's a lot!

Definitely. I took care so the changes are mostly reviewable by commit. Exceptions are the type signatures of HOCs and the tests. Those took some refinement over many of the later commits.

[WilliamKelley]

Fixed TS excluding null from the returns types of Meteor.userId / Meteor.user and needing to manually annotate that. The issue was turning on a compiler flag for "strict null checking" or the "strict" compiler option in general. Apparently it's recommended, but opt-in behavior.

[WilliamKelley]

Removed module augmentation for Meteor.loggingOut -- turns out it is documented in @types/meteor, I guess my IDE was just lagging.

I updated WithUserProps to be a null union with Meteor.User

---

I also made a PR to include Meteor.loggingOut in Meteor's online API docs. I will update the JSDoc summary with the link once/if merged.

[CaptainN]

I'll try to get to this soon. I haven't forgotten about it.

[WilliamKelley]

No problem.

For pre-review consideration,

• approving the removal of HOC's can cut this PR by half
• using useTracker can cut the code by 3/4 probably.

Additionally, I'm conflicted on the appropriate-ness of my tests. They pretty much just test underlying behaviors. However when the function boils down to a simple piping of output, then there's not really a unit test to write that doesnt mean stubbing everything or feeling like results aren't manufactured. If you want the tests to look any other way, I'm totally on board. However these can still be useful to illustrate typical lifecycles for consumers.

[CaptainN]

I took a pretty close look at this - it all looks great! Some of the TS stuff also looks good - worth noting - we don't use that for anything... Atmosphere doesn't support TS yet. But you could generate types and submit them to definitely typed if you like.

As far as removing the HoCs - IDK, they are already written, and I doubt they take up much space, but I'd be fine with it. Let's get the opinion of some of the others though before taking it out.

I say we ship this. Thanks for all the work! This looks great.

[StorytellerCZ]

I concur with @CaptainN, especially in regards to HOCs. They exist, they are being used and not going anywhere. Let's not break backward compatibility here needlessly.

[CaptainN]

I'm going to merge this to the other PR - and we can finish it up there.