Get getQueryString to expect relative URLs too #50

laziob · 2021-05-12T13:33:30Z

Change-type: Minor
Signed-off-by: Ezequiel Boehler [email protected]

laziob · 2021-05-12T14:09:30Z

@gelbal @pranasziaukas I added the logic to handle both relative and absolute Urls and corresponding tests that seem to be working. But as always, please give it a look since you might come up with a better way to handle this or notice something I missed.

Thanks!

gelbal · 2021-05-12T15:55:58Z

@laziob I see balenaCI returns failed because of not-prettified code. Can you please run npm run prettify and then commit again to clear these errors?

Error: File src/url-params.ts hasn't been formatted with prettier
Error: File test/url-params.test.ts hasn't been formatted with prettier

I wonder if it makes sense to include prettify in lint to catch these issues earlier 🤔
(I'm reviewing the rest of PR now)

gelbal

Good stuff pushing this forward Ezequiel. I typed couple of suggestions and discussion points.

gelbal · 2021-05-12T16:18:09Z

src/url-params.ts

+		}
+
+		// If its not a relative URL string, we make sure if its a string or an URL. It its a string, we convert it to an URL object.
+		if (typeof destinationUrl === 'string') {


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 { .... }

@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.

@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? : )

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

@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.

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.

@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.

@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!

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).

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

src/url-params.ts

gelbal · 2021-05-12T16:23:54Z

src/url-params.ts

+
+		// If its not a relative URL string, we make sure if its a string or an URL. It its a string, we convert it to an URL object.
+		if (typeof destinationUrl === 'string') {
+			destinationUrl = new URL(destinationUrl);


We need to handle the errors here when the incoming string cannot be translated to a URL. I'm not sure if relativeRegex ensures that the string with no match is actually a URL.

@gelbal Ohh yeap you are right. let me look into this.

gelbal · 2021-05-13T16:13:33Z

src/url-params.ts

+					? destinationDomainMatch.toString()
+					: null;
+			} catch (err) {
+				return ''; //I dont know what we should do when an error like this happens. Any ideas??


Good question. We could do console.log(err) but I don't know how much that'd help. I think it's fine to at least catch it as you do here.

@thgreasi @iamsolankiamit any suggestions on handling such caught errors in modules?

It's OK to silently fail here but I wonder what's the best practice.

thgreasi · 2021-06-04T17:04:29Z

src/url-params.ts

 		// 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;


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;

thgreasi · 2021-06-04T17:04:32Z

src/url-params.ts

+		let destinationDomainMatch;
+		let destinationDomain;
+		let actualDomainMatch;
+		let actualDomain;


All these are typed as any, which isn't good...
After the changes suggested bellow this will be:

Suggested change

let destinationDomainMatch;

let destinationDomain;

let actualDomainMatch;

let actualDomain;

let destinationDomain: string | undefined;

but I would suggest to push it lower, near the place it's first assinged.

thgreasi · 2021-06-04T17:06:00Z

src/url-params.ts

-		const destinationDomainMatch = destinationUrl
-			? destinationUrl.hostname.match(regex)
-			: null;
+		actualDomain = actualDomainMatch ? actualDomainMatch.toString() : null;


Suggested change

actualDomain = actualDomainMatch ? actualDomainMatch.toString() : null;

const actualDomain = actualDomainMatch ? actualDomainMatch.toString() : null;

and if you have recent enough TS in this project, then you can do:

const actualDomain = actualDomainMatch?.[0]; // this is inferred as `string | null` automatically

prefer explicitly reading the first match, that relying on toString().

thgreasi · 2021-06-04T17:10:56Z

src/url-params.ts

+		if (typeof destinationUrl === 'string') {
+			try {
+				destinationUrl = new URL(destinationUrl);
+				destinationDomainMatch = destinationUrl


Suggested change

destinationDomainMatch = destinationUrl

const destinationDomainMatch = destinationUrl

Just declare it in this scope, since this is only used in this limited scope as a temp variable and its value isn't useful outside this context.

thgreasi · 2021-06-04T17:11:13Z

src/url-params.ts

+				return ''; //I dont know what we should do when an error like this happens. Any ideas??
+			}
+		} else {
+			destinationDomainMatch = destinationUrl


Suggested change

destinationDomainMatch = destinationUrl

const destinationDomainMatch = destinationUrl

Just declare it in this scope, since this is only used in this limited scope as a temp variable and its value isn't useful outside this context.

thgreasi · 2021-06-04T17:11:31Z

src/url-params.ts

+				destinationDomain = destinationDomainMatch
+					? destinationDomainMatch.toString()
+					: null;


Suggested change

destinationDomain = destinationDomainMatch

? destinationDomainMatch.toString()

: null;

destinationDomain = destinationDomainMatch?.[0];

thgreasi · 2021-06-04T17:11:45Z

src/url-params.ts

+			destinationDomain = destinationDomainMatch
+				? destinationDomainMatch.toString()
+				: null;


Suggested change

destinationDomain = destinationDomainMatch

? destinationDomainMatch.toString()

: null;

destinationDomain = destinationDomainMatch?.[0];

laziob · 2021-06-23T16:40:15Z

@gelbal I think I properly reverted to the previous way we handled the relative URLs, hadnling string first and so but keeping some error handling. The unit tests are still working properly too. So please take a look when possible. On the other hand, I still dont know whats the best way to pass href and window.location.hostname as arguments to getQueryString() on the resin-site side here which is adressed on the PR though but with no solution yet

gelbal

Good stuff @laziob. I like this version the best.

The error you get is:
Error: File src/url-params.ts hasn't been formatted with prettier

Can you please call npm run prettify and squash all your commits before pushing once more?

gelbal · 2021-06-24T09:52:42Z

src/url-params.ts

+			return '';
+		}
+
+		// If its not a relative URL string, we make sure if its a string or an URL. It its a string, we convert it to an URL object.


I don't think we need a comment here as the code is self explanatory.

gelbal · 2021-06-24T09:53:41Z

src/url-params.ts

+					? destinationDomainMatch.toString()
+					: null;
+			} catch (err) {
+				return ''; //I dont know what we should do when an error like this happens. Any ideas??


@thgreasi @iamsolankiamit any suggestions on handling such caught errors in modules?

It's OK to silently fail here but I wonder what's the best practice.

Change-type: Minor Signed-off-by: Ezequiel Boehler [email protected]

gelbal · 2021-06-29T16:50:23Z

Hey @laziob, clicking on the Details button next to the checks takes you to the logs of balena CI. We see the following lint error:

src/url-params.ts:192:18
ERROR: 192:18  comment-format  comment must start with a space

It's best to get rid of that comment anyhow. Can you please add console.error(err) before returning?

Exposing the error seems more useful than harmful.

On other news, is this PR waiting for some other changes?

…a-io-modules/analytics-client into getQueryString_expect_relative_url

laziob · 2021-08-13T15:50:20Z

@balena-ci I self certify!

Approval reviews not allowed in Draft PRs

laziob added the versionbot/pr-draft Draft PR - Don't merge this PR automatically label May 12, 2021

laziob requested review from gelbal and pranasziaukas May 12, 2021 13:33

gelbal reviewed May 12, 2021

View reviewed changes

gelbal reviewed May 13, 2021

View reviewed changes

thgreasi reviewed Jun 4, 2021

View reviewed changes

laziob force-pushed the getQueryString_expect_relative_url branch from dd34960 to c6811c1 Compare June 23, 2021 16:37

gelbal reviewed Jun 24, 2021

View reviewed changes

Get getQueryString to expect relative URLs too

43e9be3

Change-type: Minor Signed-off-by: Ezequiel Boehler [email protected]

laziob force-pushed the getQueryString_expect_relative_url branch from c6811c1 to 43e9be3 Compare June 24, 2021 15:32

laziob requested a review from gelbal June 28, 2021 12:44

laziob added 4 commits August 13, 2021 10:29

log error before returning

652d253

added node types dependency

8faf51b

prettified

ef48d4d

Merge branch 'getQueryString_expect_relative_url' of github.com:balen…

52e2284

…a-io-modules/analytics-client into getQueryString_expect_relative_url

ghost previously approved these changes Aug 13, 2021

View reviewed changes

ghost merged commit 3066820 into master Aug 13, 2021

ghost deleted the getQueryString_expect_relative_url branch August 13, 2021 15:50

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get getQueryString to expect relative URLs too #50

Get getQueryString to expect relative URLs too #50

laziob commented May 12, 2021

laziob commented May 12, 2021

gelbal commented May 12, 2021

gelbal left a comment

gelbal May 12, 2021

laziob May 12, 2021

gelbal May 13, 2021

gelbal May 13, 2021 •

edited

Loading

laziob May 13, 2021

laziob May 14, 2021

laziob May 20, 2021

laziob May 20, 2021

gelbal Jun 7, 2021 •

edited

Loading

gelbal Jun 7, 2021

gelbal May 12, 2021

laziob May 12, 2021

gelbal May 13, 2021

gelbal Jun 24, 2021

thgreasi Jun 4, 2021

thgreasi Jun 4, 2021

thgreasi Jun 4, 2021

thgreasi Jun 4, 2021

thgreasi Jun 4, 2021

thgreasi Jun 4, 2021

thgreasi Jun 4, 2021

laziob commented Jun 23, 2021

gelbal left a comment

gelbal Jun 24, 2021

gelbal Jun 24, 2021

gelbal commented Jun 29, 2021

laziob commented Aug 13, 2021

	actualDomain = actualDomainMatch ? actualDomainMatch.toString() : null;
	const actualDomain = actualDomainMatch ? actualDomainMatch.toString() : null;

	destinationDomainMatch = destinationUrl
	const destinationDomainMatch = destinationUrl

Get getQueryString to expect relative URLs too #50

Get getQueryString to expect relative URLs too #50

Conversation

laziob commented May 12, 2021

laziob commented May 12, 2021

gelbal commented May 12, 2021

gelbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gelbal May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gelbal Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laziob commented Jun 23, 2021

gelbal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gelbal commented Jun 29, 2021

laziob commented Aug 13, 2021

gelbal May 13, 2021 •

edited

Loading

gelbal Jun 7, 2021 •

edited

Loading