updates files to typescript and related test files to use them #109

smartyandrewb · 2025-07-22T17:14:24Z

clickup: https://app.clickup.com/t/86b5a6amb

camiblanch · 2025-07-23T17:09:15Z

src/SigningSender.js

-const SharedCredentials = require("./SharedCredentials");
+let SharedCredentials;
+try {
+	SharedCredentials = require("../dist/cjs/SharedCredentials.cjs").default;


I don't think this is going to work for production level stuff. None of the code in src should reference the code in dist. It seems like we are trying to get things to work in our uncompiled code with both commonjs and esmodules.

I think it's more meant that the uncompiled code should all be one or the other. I'm not sure if things are playing nicely with the hybrid solution we have. I think we need to migrate completely to esmodules to avoid having to do stuff like you are here. What do you think?

Otherwise we could maybe update our tests to all run on compiled code only? I'm not sure if that's the conventional way of doing things though

camiblanch · 2025-07-30T21:45:57Z

tests/test_AgentSender.ts

@@ -0,0 +1,24 @@
+import { expect } from "chai";
+import * as SmartySDK from "../dist/esm/index.mjs";


I still don't love that we are referencing files from the /dist/ directory in the tests. We should be able to setup the tests to run on the non bundled code. I think the only exception we've made is in the tests/test_ExtractExample.js file because it helps us to test that our esm translation/bundling happens correctly. If we can avoid doing it in other places that would be great.

I think an additional task outside of this project could be to refactor the tests so there's a test specifically written for testing that esm translation/bundling is happening correctly instead of having it as part of the extract tests

camiblanch · 2025-07-30T22:43:52Z

tests/test_RetrySender.js

 const expect = chai.expect;
-const RetrySender = require("../src/RetrySender");
-const {MockSenderWithStatusCodesAndHeaders} = require("./fixtures/mock_senders");
+const RetrySender = require("../dist/cjs/RetrySender.cjs").default;


Instead of doing this, can we convert this test file to typescript like you did with a bunch of the other tests? Then you can use import instead of require here and avoid referencing the /dist directory

camiblanch · 2025-07-30T22:56:16Z

tests/test_RetrySender.js


 	it("test backoff does not exceed max", async function () {
-		let inner = new MockSenderWithStatusCodesAndHeaders(["500", "500", "500", "500", "500", "500", "500", "500", "500", "500", "500", "500", "500", "200"]);
+		let inner = new MockSenderWithStatusCodesAndHeaders([


Was this change due to prettier? Maybe we should add the prettier config to this repo as well so we don't just go with the default prettier settings?

camiblanch · 2025-07-30T22:58:02Z

tsconfig.json

+    "types": ["mocha", "chai", "node"]
  },
-  "include": ["src/**/*"],
+  "include": ["src/**/*", "tests/**/*.ts"],


I'm not super well versed in how the tsconfig.json should be setup, but I'm wondering if this could be simplified to include all test files like we do for the src files? So instead of *.ts change it to *?

camiblanch · 2025-07-30T22:58:29Z

tests/test_BaseUrlSender.ts

-	let request;
-	let urlOverride;
-	let baseUrlSender;
+	let innerSender: Sender;


Nice capturing these common types

camiblanch · 2025-07-30T23:08:16Z

index.mjs

-  SharedCredentials,
-  StaticCredentials,
-  Errors,
+	AgentSender,


I don't think that the AgentSender and Request classes should be exposed as part of the core. Our end users shouldn't be calling those directly so they shouldn't be included in core.

…o ts

camiblanch · 2025-08-04T16:19:08Z

tests/test_AgentSender.ts

+import AgentSender from "../src/AgentSender.js";
+import Request from "../src/Request.js";
+import Response from "../src/Response.js";
+import { Sender } from "../src/types.js";


Suggested change

import { Sender } from "../src/types.js";

import { Sender } from "../src/types";

Since it's a typescript file, you can omit the extension according to the tsconfig

camiblanch · 2025-08-04T16:20:57Z

tests/test_BaseUrlSender.ts

+import BaseUrlSender from "../src/BaseUrlSender.js";
+import Request from "../src/Request.js";
+import Response from "../src/Response.js";
+import { Sender } from "../src/types.js";


Suggested change

import { Sender } from "../src/types.js";

import { Sender } from "../src/types";

camiblanch · 2025-08-04T16:21:19Z

tests/test_CustomHeaderSender.ts

+import CustomHeaderSender from "../src/CustomHeaderSender.js";
+import Request from "../src/Request.js";
+import Response from "../src/Response.js";
+import { Sender } from "../src/types.js";


Suggested change

import { Sender } from "../src/types.js";

import { Sender } from "../src/types";

camiblanch · 2025-08-04T16:22:16Z

tests/test_RetrySender.ts

+import { MockSenderWithStatusCodesAndHeaders } from "./fixtures/mock_senders.js";
+import Request from "../src/Request.js";
+import Response from "../src/Response.js";
+import type { Sender, Sleeper, MockSenderInstance, MockSleeperInstance } from "../src/types.js";


Suggested change

import type { Sender, Sleeper, MockSenderInstance, MockSleeperInstance } from "../src/types.js";

import type { Sender, Sleeper, MockSenderInstance, MockSleeperInstance } from "../src/types";

camiblanch · 2025-08-04T16:23:36Z

tests/test_SigningSender.ts

+import SigningSender from "../src/SigningSender.js";
+import StaticCredentials from "../src/StaticCredentials.js";
+import SharedCredentials from "../src/SharedCredentials.js";
+import { UnprocessableEntityError } from "../src/Errors.js";


Suggested change

import { UnprocessableEntityError } from "../src/Errors.js";

import { UnprocessableEntityError } from "../src/Errors";

camiblanch · 2025-08-04T16:56:55Z

src/types.ts

+}
+
+export interface Response {
+	statusCode: string | number;


Could this be always a number instead of sometimes a string?

camiblanch

Looks great!

updates files to typescript and related test files to use them

f7b5f75

smartyandrewb requested a review from camiblanch July 22, 2025 17:24

camiblanch requested changes Jul 23, 2025

View reviewed changes

smartyandrewb added 6 commits July 29, 2025 08:53

converts SigningSender to typescript

2269d1b

fixes try catch and converts more test files to typescript

acb68c3

fixes cjs import

6343094

fixes error

36093d8

another try

1a1722f

fixes more cjs import

828334d

camiblanch requested changes Jul 30, 2025

View reviewed changes

smartyandrewb added 2 commits July 31, 2025 10:50

fixes dist/ imports, adds prettier config, converts more test files t…

5f83cb7

…o ts

updates test_RetrySender to typescript

545c7e3

camiblanch requested changes Aug 4, 2025

View reviewed changes

fixes file extensions and response statusCode type

209eb4c

camiblanch approved these changes Aug 5, 2025

View reviewed changes

smartyandrewb merged commit fad7794 into master Aug 5, 2025
3 checks passed

		@@ -0,0 +1,24 @@
		import { expect } from "chai";
		import * as SmartySDK from "../dist/esm/index.mjs";

	import { Sender } from "../src/types.js";
	import { Sender } from "../src/types";

	import type { Sender, Sleeper, MockSenderInstance, MockSleeperInstance } from "../src/types.js";
	import type { Sender, Sleeper, MockSenderInstance, MockSleeperInstance } from "../src/types";

	import { UnprocessableEntityError } from "../src/Errors.js";
	import { UnprocessableEntityError } from "../src/Errors";

updates files to typescript and related test files to use them #109

updates files to typescript and related test files to use them #109

Uh oh!

Conversation

smartyandrewb commented Jul 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camiblanch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants