[MNT-25408]: updates changing search config logic; exposes stream for active filters #11386
Conversation
rmnvch
requested review from
DenysVuika,
eromano and
popovicsandras
as code owners
lib/content-services/src/lib/search/services/base-query-builder.service.ts
Outdated
Show resolved
Hide resolved
| }); | ||
| } | ||
|
|
||
| private readonly queryFragmentsHandler: ProxyHandler<{ [key: string]: any }> = { |
There was a problem hiding this comment.
queryFragmentsHandler is private readonly field. It's not a function. So please move it above class to place where all fields are placed
lib/content-services/src/lib/search/services/search-query-builder.service.spec.ts
Outdated
Show resolved
Hide resolved
| return this._queryFragments; | ||
| } | ||
|
|
||
| set queryFragments(value: { [key: string]: any }) { |
There was a problem hiding this comment.
Two things:
- Can we set string instead of any because I see that queryFragments field has string as value type (right side)? If it works then it would be better option
- If no then see following
Any here and in setter below give eslint issue. I know you don't know what the type will be as they can have any type but can we use different type than any like "uknown" or "never"? They are a bit safer alternatives
https://rahuulmiishra.medium.com/typescripts-any-vs-unknown-vs-never-when-and-how-to-use-them-35d81ea57c01
Would it have sense to use one of those types?
There was a problem hiding this comment.
@AleksanderSklorz , minding that it's a library and type might be other than string, "unknown" would be the right choice. I've checked and all over the code inside of ADF we just once assign undefined there instead of string and that wouldn't be a problem. The problem I see is that we cannot just change it to 'unknown' because it would be a breaking change - some consumers would need type narrowing now, so I believe it should be supported by a sort of migration guide. Probably it should be a dedicated ticket for that
There was a problem hiding this comment.
Let me ask Michał what steps we should do in that case. @MichalKinas can you help us make decision? You can check that comment and open comments below which are related
There was a problem hiding this comment.
Unknow may throw some TS issues when compared with any other type so I would avoid it in that case, since it can take some other types except of string I think any usage would be justified here, we can think of suppresing the eslint warning there too
lib/content-services/src/lib/search/services/base-query-builder.service.ts
Show resolved
Hide resolved
lib/content-services/src/lib/search/services/base-query-builder.service.ts
Show resolved
Hide resolved
lib/content-services/src/lib/search/services/base-query-builder.service.ts
Outdated
Show resolved
Hide resolved
lib/content-services/src/lib/search/services/base-query-builder.service.ts
Show resolved
Hide resolved
lib/content-services/src/lib/search/services/base-query-builder.service.ts
Outdated
Show resolved
Hide resolved
|
rmnvch
requested review from
dominikiwanekhyland,
g-jaskowski and
nikita-web-ua
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
https://hyland.atlassian.net/browse/MNT-25408
What is the new behaviour?
Exposes 2 streams to watch filter change; adds Proxy to detect direct property assignment for BaseQueryBuilderService's queryFragments
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: