Skip to content

Get getQueryString to expect relative URLs too #50

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
5 commits merged into from
Aug 13, 2021
Merged

Get getQueryString to expect relative URLs too #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
43e9be3
Get getQueryString to expect relative URLs too
laziob May 12, 2021
652d253
log error before returning
laziob Aug 13, 2021
8faf51b
added node types dependency
laziob Aug 13, 2021
ef48d4d
prettified
laziob Aug 13, 2021
52e2284
Merge branch 'getQueryString_expect_relative_url' of github.com:balen…
laziob Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@types/js-cookie": "^2.2.5",
"@types/lodash": "^4.14.142",
"@types/mixpanel-browser": "2.35.1",
"@types/node": "^16.6.1",
"husky": "^3.0.9",
"jest": "^24.9.0",
"lint-staged": "^9.4.2",
Expand Down
38 changes: 26 additions & 12 deletions src/url-params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,35 @@ export class AnalyticsUrlParams {
/**
* @return full query parameter string that can be appended to URLs
*/
getQueryString(destinationUrl?: URL, currentUrl?: URL): string {
getQueryString(destinationUrl?: URL | string, currentUrl?: URL): string {
// we first check if destionationUrl is a relative URL string. If it is, we exit.
const relativeRegex = /^(?!www\.|(?:http|ftp)s?:\/\/|[A-Za-z]:\\|\/\/).*/g;
if (
typeof destinationUrl === 'string' &&
destinationUrl.match(relativeRegex)
) {
return '';
}

if (typeof destinationUrl === 'string') {
Copy link
Contributor

@gelbal gelbal May 12, 2021

Choose a reason for hiding this comment

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

You can combine this if statement with the one above to eliminate the double check for string. The code and comments will also read better.

if (typeof destinationUrl === 'string') {
    if (destinationUrl.match(relativeRegex)) {
        return '';
    } else {
        ....
}

Copy link
Contributor Author

@laziob laziob May 12, 2021

Choose a reason for hiding this comment

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

@gelbal Lol I was going to do it that way first, but for some reason I remembered that once I saw the if statements one below the other and that that was the usual practice or something like that and went the other way. Sure I'll change that.

Copy link
Contributor

@gelbal gelbal May 13, 2021

Choose a reason for hiding this comment

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

@laziob oh I see you made a major rework. Actually I liked your previous implementation as it was cleaner (once you combine the new if statements). So the code handles the string scenario first and the proceeds to handling the URL scenario. Can you please bring it back? : )

Copy link
Contributor

@gelbal gelbal May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Petros (ex-CTO) made a pretty good presentation about such design principle actually (he calls it normalizing if). I highly recommend listening to it:
https://docs.google.com/presentation/d/1hFV5YBL4FflfN7OIJr6eZrUV4Pk158wOXWzea1ANTnM/edit#slide=id.p
https://drive.google.com/file/d/1xtHev7HUwbASdasf8cWfG9O2oHdlXX43/view

Copy link
Contributor Author

@laziob laziob May 13, 2021

Choose a reason for hiding this comment

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

@gelbal It still handles the string scenario first. The code above is for the other parameter that stays that way regardless of the destinationUrl. The reason I changed it was because with the previous design, I had to repeat much more code, once for each if and even then couldnt make it work properly. In fact I dont even like this design, since it still repeats code. But it repeated less than the previous one. I still also dont believe we should be handling relative URLs through this method. It kind of undermines the use of TypeScript and gives place to the bad use of the method in resin-site, since I still think they shoudnt be forcing this method in every link "just in case". Performance wise it doesnt make sense to me. resin-site should have a better way to identify outbound links from inbound links, and call this method only on outbound links.

Having said that, I could try go back to the previous design, but I could make it work with the previous design. This was so far the only way I achieved what I wanted lol.

Copy link
Contributor Author

@laziob laziob May 14, 2021

Choose a reason for hiding this comment

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

To put an example, Hub handles it like this:

deployUrl = getExternalLinkWithTracking( 'https://dashboard.balena-cloud.com/deploy?repoUrl=${repoUrl}')

So it explicitly uses the method on the only domain where its needed. For example marketing site only needs to use it on the Login and Signup buttons, which exist only in 2 places, the header (for most of the resin-site pages) , like here and the box for on the landing page.

So its not that they need to detect relative urls in a simple way. Its that they have to detect URLs to another balena domain, not even including subdomains.

If using a list is the best way to do it I dont really know, but if its needed to be raised for further discussion Im up for that. Whats the proper way and place to raise it? ALthough It just seems too much for the task at hand tbh.

Copy link
Contributor Author

@laziob laziob May 20, 2021

Choose a reason for hiding this comment

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

@gelbal we need to work on this soon since its the only thing preventing marketing sites to update to the last analytics client. Its still using 0.12 so its not stiching sessions nor using the last referrer updates.

Copy link
Contributor Author

@laziob laziob May 20, 2021

Choose a reason for hiding this comment

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

@iamsolankiamit @thgreasi @JSReds if you could take a look into the discussion too so we can decide on how we advance it would be really helpful and appreciated! thanks!

Copy link
Contributor

@gelbal gelbal Jun 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @laziob cc @thgreasi @JSReds @iamsolankiamit, how about introducing an optional parameter (in the analytics-client constructor) for a list of domains to track?

We could retrieve this list either directly from the client that calls analytics-client or indirectly from the projectName configuration stored in the analytics-backend. At first it'll have the following content:

*.balena.io
*.balena-cloud.com

We will probably end up updating this list seldom so it's not a big maintenance issue to store it on the analytics-backend side.

Edit: Thinking it further, part of my suggestion actually means that analytics-client needs to retrieve such stateful information from the analytics-backend at each initialization. So maybe that's a no-go. It's cleaner for the component to configure analytics-client directly (without dependency on the backend).

Copy link
Contributor

@gelbal gelbal Jun 7, 2021

Choose a reason for hiding this comment

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

Having said these, let's still go ahead with working on this PR to add support for relative URL detection.

try {
destinationUrl = new URL(destinationUrl);
} catch (err) {
console.error(err);
return '';
}
}

// this regex is based on the assumption that we wont be using TLDs longer than 3 characters. If we do, it will break
// the logic and take that longer TLD as the main domain, for example hub.balena.edge.io -> edge.io
const regex = /([a-zA-Z0-9-]+)(\.[a-zA-Z]{2,3})?(\.[a-zA-Z]+$)/g;

let actualDomainMatch;
Copy link
Contributor

@thgreasi thgreasi Jun 4, 2021

Choose a reason for hiding this comment

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

I actually prefer it here, next to where it's first assigned, so that's easier to follow.
Also type it as:

let actualDomainMatch: RegExpMatchArray | null;

const destinationDomainMatch = destinationUrl
? destinationUrl.hostname.match(regex)
: null;
const destinationDomain = destinationDomainMatch?.[0];

let actualDomainMatch: RegExpMatchArray | null;
if (currentUrl) {
actualDomainMatch = currentUrl.hostname.match(regex);
} else if (typeof window !== 'undefined') {
Expand All @@ -189,16 +212,7 @@ export class AnalyticsUrlParams {
actualDomainMatch = null;
}

const destinationDomainMatch = destinationUrl
? destinationUrl.hostname.match(regex)
: null;

const actualDomain = actualDomainMatch
? actualDomainMatch.toString()
: null;
const destinationDomain = destinationDomainMatch
? destinationDomainMatch.toString()
: null;
const actualDomain = actualDomainMatch?.[0];

if (!destinationDomain || actualDomain !== destinationDomain) {
return [this.getDeviceIdsQueryString(), this.getSessionIdQueryString()]
Expand Down
21 changes: 21 additions & 0 deletions test/url-params.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,27 @@ test('parsing and matching destination and actual URL to regex', () => {
new URL('https://domain2.edge.io'),
),
).toBe('');

// Case when passing a relative URL as destinationUrl
expect(
urlParams.getQueryString('/etcher', new URL('https://domain2.edge.io')),
).toBe('');

// Case when passing an absolute URL as a string for destinationUrl and no passing is expected
expect(
urlParams.getQueryString(
'https://test.domain.io',
new URL('https://domain.io'),
),
).toBe('');

// Case when passing an absolute URL as a string for destinationUrl and passing is expected
expect(
urlParams.getQueryString(
'https://test.domain.io',
new URL('https://otherdomain.com'),
),
).toBe('d_id=d1&s_id=123');
});

interface AnalyticsMock {
Expand Down
3 changes: 3 additions & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{
"compilerOptions": {
"lib": [
"ES6","DOM"
],
"module": "commonjs",
"target": "es5",
"outDir": "dist",
Expand Down