Add URL container support to Node.JS transports

[1999]

I'm trying to use @sentry/node inside Forge runtime. Forge is Atlassian's new cloud-compute platform that developers can use to build apps for Jira, Confluence and more.

When a Forge app is invoked, the JavaScript code is executed within the app sandbox. This environment differs from a traditional Node.js environment you might already be familiar with.

The problems I had to deal with were:

• No http.Agent available - Forge runtime suggests using a higher-level custom Fetch API
• global.process.on is not a function - Forge runtime doesn't have a full global support
• TypeError: url.URL is not a constructor - Forge url doesn't contain ES6 URL API - this PR is about it.

When I looked into @sentry/node types, I noticed that BaseTransport provides a way to pass a custom "module". I built a custom ForgeTransport and it worked. But the only missing part was this piece of code from BaseTransport:

const options = this._getRequestOptions(new url.URL(sentryReq.url));

Firstly, it looked to me that new url.URL can just be replaced with new URL. But then I realised that @sentry/node is supposed to work even in Node.JS=6 which doesn't have an ES6 URL API. Therefore, I added a new property "url" to BaseTransport which behaves in a similar way to "module" and "client": transports that extend BaseTransport can override it. I updated HttpTransport and HttpsTransport. And this works for my custom ForgeTransport as well as set a custom this.url = smth property.

I also added tests. Hopefully, they are easy to read.

[1999]

@kamilogorek hey, can you please spend some time reviewing this PR?

[1999]

@HazAT hey, could you help me with reviewing this PR?

[kamilogorek]

Hey, thanks for your contribution. Regarding the PR itself, how about instead of passing url around, we change it to a more generalized urlParser function:

type URLParts = {
  hostname: string,
  pathname: string,
  port: string,
  protocol: string
};
type URLParser = (url: string) => URLParts;

where an implementation for http(s) would be as simple as:

const urlParser: URLParser = (url: string) =>  {
  const { hostname, pathname, port, protocol } = new url.Url();
  return { hostname, pathname, port, protocol };
}

and _getRequestOptions could then accept a raw string instead of new url.URL and call this.urlParser() internally.

This way Base would have no knowledge whatsoever about URL module.

[1999]

@kamilogorek sure. Does this look reasonable?

[kamilogorek]

Yes, exactly like this, thanks! Let me get a second pair of eyes on his too.

[kamilogorek]

Can we actually make this non-optional, and move the default utl => new URL(url) here? Otherwise it may be a breaking change for people using custom transports.

[1999]

@kamilogorek before I jump into this rabbit hole, is this what you mean?

diff --git packages/node/src/transports/base.ts packages/node/src/transports/base.ts
index 1b4977de..3413cbb8 100644
--- packages/node/src/transports/base.ts
+++ packages/node/src/transports/base.ts
@@ -65,9 +65,6 @@ export abstract class BaseTransport implements Transport {
   /** The Agent used for corresponding transport */
   public client?: http.Agent | https.Agent;
 
-  /** The function used to parse URLs */
-  public urlParser?: UrlParser;
-
   /** API object */
   protected _api: API;
 
@@ -82,6 +79,9 @@ export abstract class BaseTransport implements Transport {
     this._api = new API(options.dsn, options._metadata);
   }
 
+  /** Default function used to parse URLs */
+  public urlParser: UrlParser = url => new URL(url);
+
   /**
    * @inheritDoc
    */
@@ -230,9 +230,6 @@ export abstract class BaseTransport implements Transport {
         if (!this.module) {
           throw new SentryError('No module available');
         }
-        if (!this.urlParser) {
-          throw new SentryError('No URL parser configured');
-        }
         const options = this._getRequestOptions(this.urlParser(sentryReq.url));
         const req = this.module.request(options, (res: http.IncomingMessage) => {
           const statusCode = res.statusCode || 500;

I also feel like URL parser can belong as an (optional) option in TransportOptions. What do you think?

[kamilogorek]

Yes, exactly that. Tbh I don't see anyone ever wanting to override this other than use cases like yours, where completely custom transport is required.

[1999]

@kamilogorek gotcha, done.

[kamilogorek]

Thanks!