[Enterprise Search] Add reusable FlashMessages helper

[cee-chen]

Summary

Previously, most individual App Search views managed their own FlashMessages state/store. We should simplify this/DRY this out by adding a single reusable FlashMessagesLogic that other views connect to / grab its actions.setMessages() from.

This PR only provides the base logic without any current prod examples. A dummy/basic example is available here: cee-chen@9cb195d

Screencaps

Setting and clearing messages

Queuing a message and then navigating to a new page shows the queued message

QA

To test this functionality (seen in the above screencaps) locally, you can check out my flash-messages-test branch and use the 'Account Settings' / 'Credentials' links.

• Setting messages works (setMessages)
• Set messages clear when navigating to a new page
• Clearing messages with clearMessages() works
• Setting a queued message and then navigating to a new page shows the queued message

Checklist

• Unit or functional tests were updated or added to match the most common scenarios
• This renders correctly on smaller devices using a responsive layout.

[cee-chen]

@byronhulcher @scottybollinger @JasonStoltz I wanted to highlight/get your thoughts on a data structure change I'm proposing for Flash Messages. Rather than passing in individual error/success/info/warning keys, I'd like to see us treat flash messages as an array of IFlashMessage objects that look like this:

[
  {
    type: 'error', // | success | info | warning
    message: 'Something went wrong', // the main title/error text
    description: <EuiLink>Fix it here</EuiLink>, // any extra description, subtext, action, etc.
  }
]

This allows us to simplify our FlashMessages component to iterate through messages instead of having to check 4 different arrays.

This also allows us some flexibility in our flash messages - for example, (contrived hypothetical example) if in the future we need a certain flash message to persist across the entire site, we could easily add a new shouldPersist: true boolean flag to flash message objs and filter the messages array based on that property.

Let me know if you see any potential issues with this change!

[JasonStoltz]

Makes sense to me.

[scottybollinger]

No issue. Can you share an example of implementation?
Edit. I see one below:

FlashMessagesLogic.actions.setQueuedMessage({ type: 'success', message: 'Successfully deleted item' })

[cee-chen]

Here's a few more if that helps!

FlashMessagesLogic.actions.setFlashMessages([
  { type: 'success', message: 'Successfully deleted item' }
  { type: 'warning', message: 'Some other warning', description: 'A detailed message body, you can use JSX' }
  { type: 'info', message: 'This is probably too many flash messages' }
]);

You can send either an array of flash message objs or a single obj if it's just one flash message. Both setFlashMessages and setQueuedMessages work the same way.

[byronhulcher]

I like this! I think it could be useful if/when we want to start pushing rather than setting flash messages.

[cee-chen]

API doc for future reference: https://github.com/ReactTraining/history/blob/master/docs/api-reference.md#historylistenlistener-listener

[cee-chen]

I also want to quickly highlight here - the concept of queuedMessages is something @byronhulcher and I came up with a while back for App Search here.

It's most useful for deletion scenarios, where the UX flow goes as such:

• User "deletes" an item which is represented by the current page they're on (e.g. a role mapping)
• User is redirected to the parent page which shows a list of all items
• A "Deletion successful" flash message appears on the new page

In that flow, the logic/code in this new Kibana logic would look something like this:

• API call to delete item
• On success, FlashMessagesLogic.actions.setQueuedMessage({ type: 'success', message: 'Successfully deleted item' })
• Redirect user (in Kibana, navigateToUrl('/app/enterprise_search/app_search/))
• The <FlashMessages /> component now automatically handles clearing and displaying queued messages for you.

[JasonStoltz]

Nice!

[JasonStoltz]

I like the explicit distinction.

[byronhulcher]

I'm glad this has continued to prove useful!

[JasonStoltz]

This is great. Thanks.

[JasonStoltz]

Nice!

[JasonStoltz]

I like the explicit distinction.

[JasonStoltz]

Makes sense to me.

[JasonStoltz]

Nice thought.

[JasonStoltz]

What happens if you call this twice, does it replace this previously set messages?

[cee-chen]

It does yeah. I contemplated adding an addMessage() method that simply pushes / doesn't completely wipe out previous messages, but we haven't really seen a need for that so far - we definitely could in the future though if that use case popped up

[cee-chen]

@elasticmachine merge upstream

[byronhulcher]

👀 I see you referencing a type via the property of another type

[scottybollinger]

AFAICT the only thing not addressed, from my perspective, is the Logic vs noLogic and that has been addressed in Slack, pending Vadim's comment

[byronhulcher]

I think if you made this change, you wouldn't have to store historyListener in FlashMessageLogic, which doesn't seem to be used other than to round trip it here. (and for what its worth, if you are using historyListener elsewhere in FlashMessagesLogic, you should pass it in as props to the logic instead if you don't plan to be setting it often

  const { queuedMessages } = useValues(FlashMessagesLogic) as IFlashMessagesLogicValues;
  const { 
    clearFlashMessages,
    clearQueuedMessages,
    setFlashMessages
  } = useActions(FlashMessagesLogic) as IFlashMessagesLogicActions;
  
  const unlisten = history.listen(() => {
    clearFlashMessages();
    setFlashMessages(queuedMessages);
    clearQueuedMessages();
  });
      
  useEffect(() => {
    return () => unlisten(history);
  }, []);
 

(not a suggestion because I wrote this in-browser)

[cee-chen]

II think that depends on where we prefer to keep the majority of our logic - do we want it to be in the FlashMessagesLogic, or mixed in with with the <FlashMesagesProvider /> React component file?

To be honest, I do have a moderate preference to keeping it within FlashMessagesLogic. I like that all the state management is in one place and I'm not jumping between files to check how React Router is controlling flashMessages. It also makes testing easier/better, IMO - it means we can test the real FlashMessagesLogic actions and the values that change as a result, without having to either mock its import or mount the component.

Also worth noting is that the concept of storing a listener in Kea state follows the same logic that our HTTP interceptors use:

kibana/x-pack/plugins/enterprise_search/public/applications/shared/http/http_logic.ts

Lines 53 to 57 in 90bd654

	listeners: ({ values, actions }) => ({
	initializeHttpInterceptors: () => {
	const httpInterceptors = [];
	const errorConnectingInterceptor = values.http.intercept({

kibana/x-pack/plugins/enterprise_search/public/applications/shared/http/http_provider.tsx

Lines 19 to 23 in 90bd654

	export const HttpProvider: React.FC<IHttpProviderProps> = (props) => {
	const { initializeHttp, initializeHttpInterceptors } = useActions(HttpLogic) as IHttpLogicActions;
	useEffect(() => {
	initializeHttp(props);

which is a pattern I copied from how our current read-only mode interceptor works in ent-search.

[byronhulcher]

Okay I understand more. You want FlashMessagesLogic to receive history, and all logic wrt history to happen in FlashMessagesLogic. I still think there's a way to accomplish this where 1) we pass history in as a prop to an instanced kea logic instead of using a reducer and 2) the creation of the unlisten callback with history.listen, and the calling of that callback happen in FlashMessagesLogic events.beforeMount and events.afterMount.

I think, even though we are using it in a few places, "calling an action exactly once to set a reducer when a logic is first mounted. It's the difference between const spot = new Pet(); spot.setType('dog'); and const spot = new Pet({type: 'dog'});. If the Pet class required type to work, and we were never changed that property of the class after initialization, IMO it makes sense to pass in type when creating a new instance of the Pet class. This is a super reductive example, but if I have time later today I'll see if I can make an example PR of how I'd update the read only interceptor, and we can discuss the pro/cons of that approach there

[cee-chen]

I'm going to be honest, I don't fully see the distinction/point you're making here. In your analogy:

const spot = new Pet(); spot.setType('dog');

If the Pet class required type to work [...] it makes sense to pass in type when creating a new instance of the Pet class

To be clear: FlashMessagesLogic does not inherently require history to work. It requires it to work the way we are currently used to, where flash messages reset on page load. However, that behavior was an artifact/byproduct of us having different FlashMessages logic for each view.

The history listener is a shortcut or workaround to wipe messages on navigation, but a generic FlashMessages component by definition does not need that behavior. We just specifically want it to work that way for us, because that's what we're used to. There's plenty of use cases in general web app development that I can see as perfectly valid where messages should persist on navigation, and views call clearFlashMessages manually.

[cee-chen]

Similarly, HttpLogic does not inherently require interceptors to work. HttpLogic has several functions, one of which is storing Kibana's http in Kea so that other Logic files can access that lib/value without it having to be manually passed via param. Its other function is adding http interceptors/listeners, and there may be more functions later - we don't know.

The light <SomethingProvider> wrappers added for those are a way of initializing/hydrating those logic files with 3rd party deps such as http and history, but also importantly are a way of ensuring that those Kea logic files stay mounted throughout all navigation & interaction within our app.

Hopefully I'm not barking up the wrong tree or over-explaining the wrong thing here. I think it would benefit us to switch to synchronous chat or a call to clear this up, since I'd also like to clarify if you think this is a blocker to getting the PR merged.

[cee-chen]

Closing the loop: Chatted synchronously w/ Byron and this thread isn't a blocker, but we'll follow up later w/ a code example.

Will go ahead and merge this PR now - thanks Byron, and looking forward to continuing this discussion!

[byronhulcher]

@constancecchen I left some suggestions for you!

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 6481a01

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
enterpriseSearch	204	+4	200

async chunks size

id	value	diff	baseline
enterpriseSearch	356.7KB	+7.6KB	349.0KB

distributable file count

id	value	diff	baseline
total	53201	+4	53197

History

• 💚 Build #70585 succeeded ec50b60
• 💔 Build #70536 failed 0fd4000
• 💔 Build #70549 failed b31dece
• 💚 Build #70339 succeeded 54cc875
• 💔 Build #70327 failed c40d893

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[scottybollinger]

🎉