Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make LineUp v4 RegExp filter serialization backwards compatible #345

Merged
merged 26 commits into from
Apr 6, 2020

Conversation

oltionchampari
Copy link
Contributor

Close #331

Summary

  • Extended tests for serializeLineUpFilter() and restoreRegExp() functions.
  • Refactored the functions to be backwards compatible.

@oltionchampari oltionchampari added type: bug Something isn't working release: minor PR merge results in a new minor version labels Mar 31, 2020
src/lineup/internal/cmds.ts Outdated Show resolved Hide resolved
src/lineup/internal/cmds.ts Outdated Show resolved Hide resolved
src/lineup/internal/cmds.ts Outdated Show resolved Hide resolved
src/lineup/internal/cmds.ts Outdated Show resolved Hide resolved
src/lineup/internal/cmds.ts Outdated Show resolved Hide resolved
tests/lineup/internal/cmds.test.ts Outdated Show resolved Hide resolved
src/lineup/internal/cmds.ts Outdated Show resolved Hide resolved
@oltionchampari oltionchampari requested a review from thinkh April 1, 2020 07:54
@thinkh thinkh changed the title Make regex filter value serialization backwards compatible Make LineUp v4 RegExp filter serialization backwards compatible Apr 2, 2020
Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes to the file structure and added an unknown error. I'm still unsure about the typeguard function. Please have a look again. Thanks.

*
* @param filter Any value that could be a filter
*/
export function isSerializedFilter(filter: any): ISerializableLineUpFilter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function isSerializedFilter(filter: any): ISerializableLineUpFilter {
export function isSerializedFilter(filter: any): filter is ISerializableLineUpFilter {

I would expect that this is a type guard function. And the return statement indicates a boolean return value. Please revise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function checks if the filter is ISerializableLineUpFilter | ILineUpStringFilter, so i added a typeguard that includes both.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That fix was only part of the solution. The reason why you had to add ILineUpStringFilter to everything was that we were using the function isSerializedFilter in recordPropertyChange which is incorrect! Instead we must check if the provided filter is a valid ILineUpStringFilter. I refactored the code and extracted + tested the type guards.

src/lineup/internal/cmds/filter.ts Show resolved Hide resolved

}

throw new Error('Unknown LineUp filter format. Unable to restore the given filter.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this error message to prevent cases where the return value of this function is void. After playing around with this implementation I got an error with the regexp string:

ordino-lineup-unknown-filter-format

This is the error in the console:

filter.ts:125 Uncaught (in promise) Error: Unknown LineUp filter format. Unable to restore the given filter.
    at restoreLineUpFilter (filter.ts:125)
    at restoreLineUpFilter (filter.ts:121)
    at ActionNode.<anonymous> (cmds.ts:227)
    at Generator.next (<anonymous>)
    at fulfilled (tslib.es6.js:71)

@oltionchampari Can you check why calling it recursively (starting from line 121) and not returning void (without the error) and now we run into the error? In guess is that we still have an uncovered case where the three typeguards above are not working correctly.

My expectation would be that we do not get the error message and one of the typeguards catches the recursive call and returns a valid filter object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was that when you go back in the provenance graph the restoreLineUpFilter()
function gets called with the old value which gets derived from the Ranking.Thus it hasn't been serialized:

const fromGraphFilter = {filter: /abc/, filterMissing: false};
  restoreLineUpFilter(fromGraphFilter)

I added a test to cover this case and returned the filter when it's already serialized.

oltionchampari added a commit that referenced this pull request Apr 2, 2020
@thinkh
Copy link
Member

thinkh commented Apr 3, 2020

@oltionchampari I refactored the code again and wrote more type guards and tests. For me it works, but can you please check and test the solution in the browser again. If you do not find any errors or flaws we are ready to merge.

@oltionchampari
Copy link
Contributor Author

It works as expected for me too, i see no problems with it. The code is much better organized now.

@thinkh thinkh merged commit f027d02 into lineupjs4 Apr 6, 2020
@thinkh thinkh deleted the thinkh/331_string-filter-compatibility branch April 6, 2020 13:08
@thinkh thinkh mentioned this pull request Jul 30, 2020
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: minor PR merge results in a new minor version type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants