Clean up routing behaviour in ngram component #1727
Conversation
| import { BooleanFilter, DateFilter, DateFilterData, MultipleChoiceFilter } from './field-filter'; | ||
| import { of } from 'rxjs'; | ||
| import { distinct } from 'rxjs/operators'; | ||
| import { SimpleStore } from '../store/simple-store'; | ||
| import { Store } from '../store/types'; |
There was a problem hiding this comment.
This change removes some unused imports, which I noticed when I was looking at other examples for testing routing behaviour.
There was a problem hiding this comment.
The only test in this spec was the one related to the ngram related models and types.
| component.onParameterChange('size', 2); | ||
| expect(component.stopPolling$.next).not.toHaveBeenCalled(); | ||
| }) | ||
|
|
There was a problem hiding this comment.
On initialization, all values in the template will be adjusted to the values reflected in the routing parameters, which will trigger the onParameterChange method, hence the extra if condition in there to see if the parameter has actually changed.
| keysInStore = ['ngramSettings']; | ||
|
|
There was a problem hiding this comment.
I think this variable is never used?
There was a problem hiding this comment.
Yes, by the StoreSync parent.
frontend/src/app/models/ngram.ts
Outdated
| analysis: 'none', | ||
| maxDocuments: 50, | ||
| numberOfNgrams: 10, | ||
| dateField: 'date' |
There was a problem hiding this comment.
The way the date field is handled looks a bit shaky, since the corpus may not actually have a field with this name. I think this was already an issue, though. Have you checked that the visualisation still works with corpora like parliament-denmark-old?
There was a problem hiding this comment.
Yes, good point. The problem is that when the ngramParameters model is initialized, the corpus input hasn't been set yet, so we cannot set the date field. Initializing in onChanges ... if (changes.corpus) and passing the first date field in the corpus on initialization may be the best way to solve this (in practice, this only happens once, as we will have to navigate away from the ngram component in order to change the corpus). I will try that. This highlights a problem that I see with outfactoring state management to models in general: often, state management and data manipulation requires transferring knowledge about preconditions for certain states as well. So a lot of the logic tied to the data will still be tied up in the component. This could probably be solved if we did more heavy refactoring, and not use @Input anymore to transfer corpus and queryModel state, and instead keep tabs on them through observables. Then again, my intuition is that it would move the code into a less angularesque state, and would make it harder to step into as a new developer.
There was a problem hiding this comment.
Hmm, I tried and abandoned the idea to initialize ngramParameters in ngOnChanges. That seems to work in practice, but unit testing depending on change detection is a bit of a nuisance. I decided to remove the date field from the routing parameters for now: the number of corpora in which different date fields are available is so limited that I think it's not too important to include the date field in the route at this moment. We can add it once we decided & implemented a more stable solution for transferring knowledge about corpus & queryModel between components and/or models.
Close #1268. As discussed IRL, I decided to focus on the problems involved in managing routing parameters connected to the ngram component, and this fixes the multiple fetching. My personal preference is to keep the concerns for parameter changes and requests separated, i.e., not to combine them both in one model. But extending
StoreSyncand using its methods works like a charm! 🤩