Update user agent #83

WilliamBergamin · 2023-08-31T15:45:33Z

Summary

The goal of this PR is to provide slack with a more meaningful user-agent information, when users make http requests to Slack.

Previously Deno would populate this header with Deno/1.x.x describing the users Deno version, now it will be populated with Deno/1.x.x OS/<user os> deno-slack-api/<version | undefined>

user os list

Testing

The unit tests try to cover all edge cases

Manual testing is tricky since the module must be released in order to get a successful version.

To test an undefined api version use the following to start a local server

const server = Deno.listen({ port: 8080 });
console.log(`HTTP webserver running.  Access it at:  http://localhost:8080/`);

for await (const conn of server) {
  serveHttp(conn);
}

async function serveHttp(conn: Deno.Conn) {

  const httpConn = Deno.serveHttp(conn);

  for await (const requestEvent of httpConn) {
	  console.log(requestEvent.request.headers.get("user-agent"));
    requestEvent.respondWith(
      new Response(`{}`, {
        status: 200,
      }),
    );
  }
}

Then use the following to send a request

import { SlackAPI } from "https://raw.githubusercontent.com/slackapi/deno-slack-api/update-user-agent/src/mod.ts"

const client = SlackAPI("test-token",
{
	slackApiUrl: "http://localhost:8080/"
});

await client.api.test();

The command line should print out something like Deno/1.36.0 OS/darwin deno-slack-api/undefined

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

WilliamBergamin · 2023-08-31T15:46:24Z

src/api_test.ts

@@ -366,41 +368,6 @@ Deno.test("SlackAPI class", async (t) => {
  mf.uninstall();
 });

-Deno.test("serializeData helper function", async (t) => {


These tests where moves to base-client-helpers_test.ts

WilliamBergamin · 2023-08-31T15:46:56Z

src/base-client.ts

@@ -87,23 +90,3 @@ export class BaseSlackAPIClient implements BaseSlackClient {
    };
  }
 }
-
-// Serialize an object into a string so as to be compatible with x-www-form-urlencoded payloads
-export function serializeData(data: Record<string, unknown>): URLSearchParams {


This function was moved to base-client-helpers.ts

codecov · 2023-08-31T15:47:48Z

Codecov Report

Merging #83 (46ab0ba) into main (d857639) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #83   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           35        36    +1     
  Lines         1063      1091   +28     
  Branches        10        13    +3     
=========================================
+ Hits          1063      1091   +28

Files Changed	Coverage Δ
src/base-client-helpers.ts	`100.00% <100.00%> (ø)`
src/base-client.ts	`100.00% <100.00%> (ø)`
src/dev_deps.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

filmaj

Hey, looking good! Thanks for tackling this.

I left a bunch of nitpicky comments. mostly around the tests, and some questions for my own education.

One other thing we should try to look into: what does this look like for a deployed application? I'm not exactly sure how this works in the context of a bundled application. I think it would be useful to run the build hook on an app that uses a versioned deno-slack-api from deno.land and see how the code from this repo ends up getting bundled into the built bundle (if it does at all!), and try to reason through how this code would work in that context.

src/base-client-helpers_test.ts

filmaj · 2023-08-31T16:33:26Z

src/base-client-helpers_test.ts

+  );
+
+  await t.step(
+    "should return the unknown if the module version is invalid",


What's the idea in this test? How is the version invalid in the test case?

Good point, I will update the test description, this test validates that the Regex used to extract the version works properly and would return undefined if the module url path does not match deno_slack_api@

Example deno_slack_sdk

src/base-client-helpers_test.ts

filmaj · 2023-08-31T16:38:14Z

src/base-client-helpers_test.ts

+
+Deno.test(getUserAgent.name, async (t) => {
+  await t.step(
+    "should return the user agent with expected output",


I'm a nitpicker when it comes to test names, but they are important. A test name can convey a lot of information about a program's behaviour without ever having to look at code. From the perspective of a new contributor unfamiliar with the project, good test names make it easier to ramp up, making it easier to contribute.

Let's be clear what 'expected output' means - you as the developer of this feature know what that means, but me or someone else does not. I would suggest instead:

Suggested change

"should return the user agent with expected output",

"should return the user agent with deno version, OS name and undefined deno-slack-api version",

Great point, I did overlook the test names here 💯

Co-authored-by: Fil Maj <[email protected]>

WilliamBergamin · 2023-08-31T21:36:14Z

what does this look like for a deployed application? I'm not exactly sure how this works in the context of a bundled application. I think it would be useful to run the build hook on an app that uses a versioned deno-slack-api from deno.land and see how the code from this repo ends up getting bundled into the built bundle (if it does at all!), and try to reason through how this code would work in that context.

App testing

I've tested this by

Creating a new app
Importing the deno-slack-api from https://raw.githubusercontent.com/slackapi/deno-slack-api/update-user-agent/src/
Editing the deno-slack-sdk locally to import the deno-slack-api from https://raw.githubusercontent.com/slackapi/deno-slack-api/update-user-agent/src/
Importing this local deno-slack-sdk in my new app
slack run works as expected 🟢
slack deploy works as expected 🟢

Remote module testing

I've release the deno_bill_api and used the following script with the above server to get the value of the user-agent

import { SlackAPI } from "https://deno.land/x/[email protected]/mod.ts"

const client = SlackAPI("test-token",
{
	slackApiUrl: "http://localhost:8080/"
});

await client.api.test();

My locally deployed server received the following user agent
user-agent = Deno/1.36.0 OS/darwin deno-slack-api/0.0.1 🟢

Note that deno-slack-api is printed instead of deno-bill-api, this is a typo in the deno-bill-api

Bundle testing

I then used the deno bundle command on the above script to generate a bundled version of my script (fetch.bundle.js) when I execute this script (deno run --allow-net fetch.bundle.js) the user-agent received by my local server is Deno/1.36.0 OS/darwin deno-slack-api/0.0.1 🟢

Turns out in the fetch.bundle.js file the following importMeta object stores import.url information of the modules 🥇

const importMeta = {
    url: "https://deno.land/x/[email protected]/base-client-helpers.ts",
    main: false
};
const API_VERSION_REGEX = /\/deno_bill_api@(.*)\//;
function getUserAgent() {
    const userAgents = [];
    userAgents.push(`Deno/${Deno.version.deno}`);
    userAgents.push(`OS/${Deno.build.os}`);
    userAgents.push(`deno-slack-api/${_internals.getModuleVersion()}`);
    return userAgents.join(" ");
}
function getModuleVersion() {
    const url = _internals.getModuleUrl();
    if (url.host === "deno.land") {
        return url.pathname.match(API_VERSION_REGEX)?.at(1);
    }
    return undefined;
}
function getModuleUrl() {
    return new URL(importMeta.url);
}

It may be fair to assume that this behavior will also apply to users using the deno-slack-api through the deno-slack-sdk

@filmaj let me know what you think about this and if it is worth testing with an app that imports a version of deno-slack-sdk that references the deno-bill-api

filmaj · 2023-08-31T23:53:00Z

Oh man thanks for that extensive testing and deep dive! Very cool info about the import.meta bundling, that's great to know!

filmaj

Looks awesome! Well done and thank you for tackling this 🙇 ❤️ 🎉

WilliamBergamin added 9 commits August 29, 2023 16:45

Update base-client.ts

b4f7947

Update base-client.ts

25676cb

Added user-agent and tests

5b304bc

remove useless changes

d86fcb1

Add one more tests

223fb5d

created base client helpers file

9c8eb36

improve speed

8bf7a0a

Improve comment

85686dc

remove useless changes

fe61d18

WilliamBergamin added enhancement New feature or request semver:patch requires a patch version number bump labels Aug 31, 2023

WilliamBergamin self-assigned this Aug 31, 2023

WilliamBergamin requested a review from a team as a code owner August 31, 2023 15:45

WilliamBergamin commented Aug 31, 2023

View reviewed changes

filmaj requested changes Aug 31, 2023

View reviewed changes

WilliamBergamin and others added 4 commits August 31, 2023 15:24

Update src/base-client-helpers_test.ts

2821009

Co-authored-by: Fil Maj <[email protected]>

update test description

6562fca

add module to function test description

513d32d

Improve test names

46ab0ba

filmaj approved these changes Sep 1, 2023

View reviewed changes

WilliamBergamin merged commit 368e406 into main Sep 1, 2023
10 checks passed

WilliamBergamin deleted the update-user-agent branch September 1, 2023 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update user agent #83

Update user agent #83

WilliamBergamin commented Aug 31, 2023 •

edited

Loading

WilliamBergamin Aug 31, 2023

WilliamBergamin Aug 31, 2023

codecov bot commented Aug 31, 2023 •

edited

Loading

filmaj left a comment

filmaj Aug 31, 2023

WilliamBergamin Aug 31, 2023

filmaj Aug 31, 2023

WilliamBergamin Aug 31, 2023

WilliamBergamin commented Aug 31, 2023

filmaj commented Aug 31, 2023

filmaj left a comment

	"should return the user agent with expected output",
	"should return the user agent with deno version, OS name and undefined deno-slack-api version",

Update user agent #83

Update user agent #83

Conversation

WilliamBergamin commented Aug 31, 2023 • edited Loading

Summary

Testing

Requirements

WilliamBergamin Aug 31, 2023

Choose a reason for hiding this comment

WilliamBergamin Aug 31, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

filmaj left a comment

Choose a reason for hiding this comment

filmaj Aug 31, 2023

Choose a reason for hiding this comment

WilliamBergamin Aug 31, 2023

Choose a reason for hiding this comment

filmaj Aug 31, 2023

Choose a reason for hiding this comment

WilliamBergamin Aug 31, 2023

Choose a reason for hiding this comment

WilliamBergamin commented Aug 31, 2023

App testing

Remote module testing

Bundle testing

filmaj commented Aug 31, 2023

filmaj left a comment

Choose a reason for hiding this comment

WilliamBergamin commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading