Changed getQueryString to validate domains

[laziob]

Since we only want to append device id and session id when needed, that is, when navigating cross domain or cross component, now the getQueryString function checks if there is a passed destination hostname and if so it compares its main domain to the main domain of the actual URL the user is on. When a difference between the two exists, or there was no destination hostname passed it returns both d_id and s_id params. Otherwise if these 2 are the same, it returns an empty string.

Change-type: Minor Signed-off-by: Ezequiel Boehler ezequiel@balena.io

[gelbal]

Hey @iamsolankiamit here is the PR for the changes around URL parameter generation we've been discussing on FD. Your review is appreciated.

[gelbal]

Good stuff with the tests @laziob. It'd be neat to mention why all the consecutive tests in the same run returns the previously set d_id and s_id values. Furthermore, it might help to split some of the tests so they don't all share the same context.

[gelbal]

One more discussion point before we merge this: Shall we maintain a list of balena domains as a configuration option? I'd love to hear your thoughts @laziob @pranasziaukas @sradevski @iamsolankiamit.

The main advantage of knowing the domains to be tracked is we could configure analytics-client not to return query parameters on external links. In the current implementation, the component maintainer would get query params for links like twitter.com, facebook.com. Then the tradeoff is the maintenance of such list. Embedding it on analytics-client seems like a no go from product builder perspective. Maybe it could be part of that secret JSON mapping that we use to retrieve Amplitude tokens. Still, it's a burden to maintain. Alternatively, we could ask the component maintainers to only make URL Params call on internal links and not on external links. Else, analytics-client will append d_id and s_id params on twitter / facebook links.

[laziob]

@gelbal I used the same run because that run covered , at a general level, all posible scenarios, which are either no param passed, only one param passed, or both params passed. And at each instance it has its expected value. Then other possible changes are differences in values, but those functions dont make any checks on what the value should be, so I didnt see the case on tsting with values different than the '123' used there.

If there is some other context that you have in mind please let me know so I can build the proper test for it.

Regarding the domains, Im with you that keeping some kind of registry might be a burden further along the road. At the moment, for example the Hub does exactly what you mentioned. It has different classes I think and depnding if the link is an internal or external one the apply different onClick functions. While Blog I think it was, handles the onClick directly on the button with the specific needed URL.

I really cant say whats the best practice here, so looking forward to hear the others opinions!

[pranasziaukas]

Shall we maintain a list of balena domains as a configuration option?

Personally, I'm not a huge fan of maintaining such lists/mappings because it is destined to fail eventually.

Alternatively, we could ask the component maintainers to only make URL Params call on internal links and not on external links.

I think this makes sense. Right now, the only real reason to use getQueryString (that I'm aware of) is to generate context to pass to further destinations - at that point, the conscious decision to pass the parameters is already made, and the decisionmaking might as well include the internal-external domains.

In other words, we should not blindly concatenate destinationUrl + urlParams.getQueryString(destinationUrl) anyway because that can easily result in unexpected side effects. Or should we? :)

[gelbal]

@laziob thanks again for going through this PR dance. Nice work. Let's ship it.

Can you please squash your commits before we merge this?

[laziob]

@gelbal there it goes! Thank you and @pranasziaukas for all the review so far! I promise I'll get better at coding eventually