feat: add value-independent hook for setting online status message [LIBS-369]

[KaiVandivier]

Implements LIBS-369

Key features

Adds individual hooks and contexts for getting and setting the online status message. As such, consumers of useSetOnlineStatusMessage won't need to rerender when the message value changes

1. useOnlineStatusMessageValue()
2. useSetOnlineStatusMessage()

Notes

• I consider these changes a feat because the existing useOnlineStatusMessage hook is relatively unchanged in behavior, but there are now two more optimal hooks available
• These changes are backwards compatible -- consumers don't need to change to one of the new hooks
• I moved the types around a little bit
• Here are consumers that might benefit from switching to using the new hooks:
  • Aggregate Data Entry app (edit: this actually uses the value too, never mind)
  • UI Header Bar (actually... probably fine as-is, since the set function won't cause unnecessary rerenders)

Checklist

• Have written Documentation
• Has tests coverage -- the existing test works as a small integration test

[Mohammer5]

I did change the return value of onlineStatusMessage to be null instead of undefined to distinguish between "not yet set" and "missing its context provider" conditions, but I doubt think this will break anything. The existing consumers I know of (see below) don't mind

This is technically a breaking change. IMO we should always bump the major version, even when we think no lib consumer will break. In case it does affects a consumer, it'd be a PITA to get to the bottom of this..

If you agree with this, we'll have two choices: Either separate that into a separate PR and discuss that particular change itself or make this PR introduce a breaking change. (I'm in favor of the former, but in this case it's such a small thing that I don't care too much)

[Birkbjo]

I think setOnlineStatusMessage will be the default value of the context - eg. ```() => undefined```` and not undefined if there's no context in the tree? But see my comment below, not sure if this is needed.

[KaiVandivier]

Ah true - I admit that these checks are a bit of an automatic habit 🤦‍♂️ I'll remove them as you suggested in your other comment

[Birkbjo]

This could cause issues if you manually set the status to undefined. Subsequent calls to this would then throw.

So if we were to do this, I think we should prevent undefined being passed to setOnlineStatusMessage. But that would definitely be a breaking change. However, if we just want to check that it's not the default, we could initialize it to a Symbol.

But this is all rendered in the app-shell, right? So this might just create more complexity than it's worth. And calling setOnlineStatusMessage without a context is fine, since it's just a noop.

So in my opinion I think we could just remove these checks, which would work-around the "potential breaking change".

Initializing with null or undefined then doesn't matter, and was never part of the API contract. And no functionality would change. The only problem would be these checks if we kept it like this (mostly due to being able to set it as undefined through setOnlineStatusMessage.

[Birkbjo]

If we really wanted to fail early, I think it would look something like this:

const defaultOnlineStatusMessage = Symbol("defaultStatusMessage");
const OnlineStatusMessageValueContext = React.createContext<
  ReactNode | typeof defaultOnlineStatusMessage
>(defaultOnlineStatusMessage);

export const useOnlineStatusMessageValue = () => {
  const onlineStatusMessage = useContext(OnlineStatusMessageValueContext);

  if (onlineStatusMessage === defaultOnlineStatusMessage) {
    throw new Error(
      "useOnlineStatusMessageValue must be used within an OnlineStatusMessageProvider"
    );
  }
  // this should be inferred to be a ReactNode
  return onlineStatusMessage;
};

useSetOnlineStatusMessage, would be similiar but extracting the "noop" default function to variable and check for that equality.

I think this could be done without a breaking change, since you would keep the public API the same.

[KaiVandivier]

That's a solid solution, but I just went with removing the undefined checks to keep it simple -- I think you're right that the app adapter kinda guarantees the provider will be there

[dhis2-bot]

🎉 This PR is included in version 3.10.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀