Upgrade query-string to v8.1.0 | Add Wrapper function for queryString.parse() #1794
Conversation
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
| @@ -20,6 +20,7 @@ import prefixUrl from '../../utils/prefix-url'; | |||
| import { MAX_LENGTH } from '../DeepDependencies/Graph/DdgNodeContent/constants'; | |||
|
|
|||
| import { SearchQuery } from '../../types/search'; | |||
| import parseQuery from '../../utils/parseQuery.test'; | |||
There was a problem hiding this comment.
why are we importing test module into prod module?
There was a problem hiding this comment.
I think that way we won't have to worry about its code coverage. Changing it back to normal .ts file
| return result; | ||
| } | ||
|
|
||
| parseQuery.wrapper = parseQuery; |
There was a problem hiding this comment.
what is the purpose of wrapper?
There was a problem hiding this comment.
This needed to be done because the way jest works is, it needs an object out of which it could spyOn the function.
So, something like jest.spyOn(func) won't work. Instead, it has to be jest.spyOn(obj.func) notation.
Thus, the wrapper thing is to make it work with jest. I tried the former approach also, without wrapper, but it didn't work with jest.
Signed-off-by: Ansh Goyal <[email protected]>
Codecov ReportPatch coverage is 📢 Thoughts on this report? Let us know!. |
| @@ -51,6 +51,7 @@ | |||
| "src/reducers/trace.js", | |||
| "src/selectors/dependencies.js", | |||
| "src/utils/configure-store.js", | |||
| "src/utils/parseQuery.ts", | |||
There was a problem hiding this comment.
Why is this needed? Don't we pick up *ts files for linting automatically? If we don't it's a very sad state - linting should be on by default.
There was a problem hiding this comment.
Not actually. We aren't picking up *ts files automatically. We are doing it for *.tsx files.
Without this rule:
There was a problem hiding this comment.
let's rename the file to tsx then, for now. I think we need to fix this separately.
There was a problem hiding this comment.
Though, it can also be configured in tsconfig.json
{
"extends": "../../tsconfig",
- "include": ["src/**/*.tsx", "typings"]
+ "include": ["src/**/*.tsx", "src/**/*.ts", "typings"]
}
| return result; | ||
| } | ||
|
|
||
| parseQuery.wrapper = parseQuery; |
There was a problem hiding this comment.
what is the point of this .wrapper? Can't we just export the function?
There was a problem hiding this comment.
We can export is without .wrapper also. But, then it won't work with jest.spyOn
There was a problem hiding this comment.
Done! Found a better workaround 🚀
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
| [key: string]: string | null | (string | null)[]; | ||
| } | ||
|
|
||
| function parseQuery(query: string, options?: ParseOptions): { [key: string]: string | string[] } { |
There was a problem hiding this comment.
- can we throw in a unit test to maintain coverage?
- under what conditions does this return a plain array? An HTTP query string is fundamentally a dictionary.
There was a problem hiding this comment.
Yes, we can have a unit test for it. Adding it in.
Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Ansh Goyal <[email protected]>
Which problem is this PR solving?
Description of the changes
queryString.parse()function to modify its type as per our codebase.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test