Remove app.search instance, cache app.cache.searched

[askvortsov1]

Fixes Part Of #2144

Changes
See title. There's no reason for the app.search component to be saved other than to cache searched (array of previous searches), so I just moved that array out to app.cache.searched

Reviewers should focus on:
Do we even need app.search to be accessible? Looking at the code, there's 0 public API for it, and I don't understand why we'd need it to be saved in app? Can we just put the sources back into Search, and initialize it like any other component?

EDIT: Looks like this is to cache previous search queries (good idea). I'm going to move that into the global app.

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[askvortsov1]

@clarkwinkelmann as a followup to #2144 (comment), what would you like to move out into a state class? Right now, I'm envisioning:

• this.getCurrentSearch()
• this.value
• sources (not sure about this one tbh)

[askvortsov1]

Decided to leave value and sources for now, as no reason to move them

[franzliedke]

Just for clarification: In other (lower-level) components, we wouldn't want to access app within the component, right? And instead have these dependencies passed in as props?

Here it's okay because page components don't get any props passed in, and we're at the top level anyway?

[askvortsov1]

Yes. But the problem isn't that they are accessing app: that in of itself is fine. We want it to be extensible. No one's gonna instantiate an instance of Index Page somewhere random, but the lower level components might be reused by extensions in unexpected ways.

[franzliedke]

the problem isn't that they are accessing app: that in of itself is fine.

Well, it would break the unidirectional data flow we're envisioning, right?

[askvortsov1]

Not quite: My main ideal goal is that the rendered content of the app can be considered the output of a somewhat pure, deterministic function, the input of which is the current state.

This means that if a component wants to change another component, it would do so by changing the state, and not by changing the component directly.

The only element I'm unsure of (and one we're gonna need to figure out) is transitional animations / direct DOM manipulations via JQuery, but we need to take this one step at a time.

[franzliedke]

The change makes sense, good job!

Just a few minor discussion points...

[askvortsov1]

@franzliedke revisiting this now, Im actually starting to think that this might not be enough of a change. Feels like value should definitely go to the state, and the clear method.

Sources I'm torn on: I don't want inheritance trees off of these states since I view them as an intermediate step between our current situation and http://meiosis.js.org/, so I'd prefer that people extend Search. But then again, perhaps additional sources should be provided as a parameter when instantiating a SearchState? Kinda torn on this one.

[franzliedke]

Inheritance trees? Additional sources? Can you expand on those? 😉

Is that anything being done by extensions currently? If not, let's worry about it now...

[askvortsov1]

I don't want people making new classes inheriting off of the states. For sources, I meant search sources: currently a property of the Search component, but could be better in Search State tbh

[dsevillamartin]

I don't want people making new classes inheriting off of the states

To give context, I assume this is referring to what I brought up today internally about my AlertState classes that act like components (but can be extended) in 4c8af1c (my frontend rewrite branch).

These AlertState instances are passed to app.alerts.show, can be extended like components, and contain component & JSX logic.

Sasha doesn't like this approach. I understand the term "state" here isn't good.

My problem with that [implementation] is that it's not moving us towards our eventual goal of using a sensible state management system. I view all these 'State' classes as intemediaries until we're able to adopt a proper state management pattern like http://meiosis.js.org/
- @askvortsov1

[franzliedke]

Previous call sites checked for the controller supporting search, right? So we have a slight change in behavior - in theory, other places having a q query param would now be interpreted as having a current search, too. Not sure if this is a practical problem. 🤔

[askvortsov1]

Now that value is stored in SearchState, we could just return that?

[franzliedke]

That won't work - getCurrentSearch() is the fallback value called in getValue(), so you'd have an infinite loop there.

That said, what about my question? We don't check whether the current page / controller supports search anymore, right?

[askvortsov1]

I do not like the search PR
This code is more opaque than tar
This jumbled madness breaks my mind,
and while an answer we shall find,
what that shall be, I can't forsee
I do not like this, Franz Liedke.

But on a serious note, we don't but we could just move that check here and return null if the page isn't searchable?

[askvortsov1]

I'll be honest, I don't really like the naming here. I think it needs to be clearer to indicate that getCurrentSearch is whatever was searched on the last page refresh, and getValue is the current value of the search input. I feel like there's way too many methods involved in search and it could probably be clearer. Not sure where to go with it though.

[franzliedke]

For documentation purposes:

• We introduced a new SearchState base class that can be used as a prop (for the Search component) for simple search interfaces (with a local cache) that don't have global side effects.
• The new GlobalSearchState is used for the global discussion search, inherits the storage and adds global side effects (taking the initial value from the URL and reflecting changes there as well as triggering redraws) and a few methods for sorting.

I hope you like this @datitisev 🤗

[dsevillamartin]

@franzliedke Thanks 👍 🤗 ❤️

Breaking changes:

• Many methods removed from Search component
  • Moved to state that needs to be stored & passed to component
  • Use SearchState class for basic search interfaces - GlobalSearchState applies mainly to IndexPage & header search
  • Some methods have been renamed
    • getCurrentSearch -> getInitialSearch
• Instead of using searching method in Page components where we want searching to apply (e.g. IndexPage), set static property hasSearchResults providesInitialSearch to true
• Perhaps a few more, depending on what your code does with Search exactly