Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Revert "Revert "Handle query values as objects for cached parsed urls""
Browse files Browse the repository at this point in the history 
This reverts commit 77873c3.
  • Loading branch information
@diracdeltas @bsclifton
diracdeltas authored and bsclifton committed Jun 26, 2017
1 parent cf6e22f commit d142b24
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions app/common/lib/siteSuggestions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ const tokenizeInput = (data) => {
parts = parts.concat(parsedUrl.pathname.split(/[.\s\\/]/))
}
if (parsedUrl.query) {
// I think fast-url-parser has a bug in it where it returns an object for query
// instead of a string. Object is supported but only when you pass in true
// for the second value of parse which we don't do.
// We can remove this when we change away from using fast-url-parser in favour of the
// Chrome URL parser.
if (parsedUrl.query.constructor !== String) {
parsedUrl.query = Object.entries(parsedUrl.query).map((x) => x.join('=')).join('&')
}
parts = parts.concat(parsedUrl.query.split(/[&=]/))
}
if (parsedUrl.protocol) {
Expand Down

3 comments on commit d142b24

@bridiver
Copy link
Collaborator

Choose a reason for hiding this comment

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

the problem here is that urlParse only returns a copy when the url is already cached. If this block gets a url that is not in the cache it will modify the cached object and all subsequent calls will get the modified query attribute. I'm going to commit a change to always return a copy, but in general I think we should avoid modifying objects that are returned from other methods unless we are intentionally creating side-effects

@bridiver
Copy link
Collaborator

@bridiver bridiver commented on d142b24 Sep 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this can also happen because of https://github.com/brave/browser-laptop/blob/master/app/ledger.js#L1368

@bridiver
Copy link
Collaborator

@bridiver bridiver commented on d142b24 Sep 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

actually I guess ledger.js is the sole cause in this case because there wasn't an assignment before this block was added

Please sign in to comment.