-
Notifications
You must be signed in to change notification settings - Fork 58
Remove destructuring from generateRequestUrl signature #363
Remove destructuring from generateRequestUrl signature #363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the CI is failing due to some sort of TypeScript error. Not entirely sure why but we should find out what is going on.
query = new UrlSearchParams(query).toString(); | ||
if (cacheBust) { | ||
export function generateRequestUrl(url: string, options: RequestOptions = {}): string { | ||
let query = new UrlSearchParams(options.query).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just move the destructering down?
export function generateRequestUrl(url: string, options: RequestOptions = {}): string {
let { query, cacheBust } = options;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it for a couple reasons:
- I thought it'd be weird to set
query
and then set it again on the line after it.
let { query, cacheBust } = options;
query = new UrlSearchParams(query).toString();
- I didn't want
cacheBust
to be alet
variable since it's not modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, ok, and it is only accessed once (cacheBust
) 👍
It looks like the errors are related to errors in functional tests, and not to changes in this PR.
I'm happy to address, but I can't reproduce locally. |
Found the issue and fixed it in #364. I am not sure why it wasn't reproducible on your machine though. |
Description:
The
generateRequestUrl
function is correct, but its usage of destructuring with a default argument is throwing an error in the way that ts-loader reads the code. Issue TypeStrong/ts-loader#442 describes the issue, but it hasn't really had much progress, and it's easier to just change this.