Auto generate index.d.ts by typescript #238

m2rads · 2025-01-02T01:46:59Z

This PR addresses issue #231

Changes

Update build:types script to generate types using tsc
Refactor index.ts to import expect
Update test files to reflect the new changes
Update readme file

…e JestEexpect

vercel · 2025-01-02T01:47:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
shortest	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 2, 2025 2:01am

m2rads

Added self-review. I can further cleanup the types to improve DX further.

m2rads · 2025-01-02T01:48:15Z

README.md

@@ -55,7 +55,7 @@ yarn shortest
    headless: false,
    baseUrl: 'http://localhost:3000',
    testPattern: '**/*.test.ts',
-    anthropicKey: process.env.ANTHROPIC_API_KEY
+    anthropicKey: process.env.ANTHROPIC_API_KEY || '',


This is now necessary as typescript is considering env vars as string | undefined therefore we have to add an fallback empty string to silence typescript errors. As mentioned before we can have these managed internally without having users to declare these here.

m2rads · 2025-01-02T01:49:14Z

README.md

@@ -72,7 +72,7 @@ You can also use callback functions to add additional assertions and other logic
 execution in browser is completed.

 ```typescript
-import { shortest } from '@antiwork/shortest';
+import { shortest, expect } from '@antiwork/shortest';


One of the main reasons why we created index.d.ts manually was to allow users to use jestExpect without having to import it. With the new changes this is necessary.

m2rads · 2025-01-02T01:49:51Z

packages/shortest/package.json

@@ -5,28 +5,27 @@
  "type": "module",
  "main": "./dist/index.js",
  "module": "./dist/index.js",
-  "types": "./index.d.ts",
+  "types": "./dist/types/index.d.ts",


reading the autogenerated types instead of the file we created manually

m2rads · 2025-01-02T01:50:11Z

packages/shortest/package.json

  ],
  "scripts": {
    "build": "rm -rf dist && pnpm build:types && pnpm build:js && pnpm build:cli",
    "prepare": "pnpm build",
    "prepublishOnly": "pnpm build",
    "postinstall": "node -e \"if (process.platform !== 'win32') { try { require('child_process').execSync('chmod +x dist/cli/bin.js') } catch (_) {} }\"",
-    "build:types": "tsc --emitDeclarationOnly --outDir dist/types && cp index.d.ts dist/",
+    "build:types": "tsc --emitDeclarationOnly --outDir dist/types",


This script is in charge of generating the types.

m2rads · 2025-01-02T01:50:29Z

packages/shortest/src/index.ts

@@ -227,6 +224,7 @@ export const test: TestAPI = Object.assign(
  },
 );

+export const expect = jestExpect;


Making jestExpect available as an import in the test files

m2rads · 2025-01-02T01:51:20Z

packages/shortest/src/types/globals.ts

+
+declare global {
+  namespace NodeJS {
+    interface Global {
+      __shortest__: ShortestGlobals;
+    }
+  }
+}
+
+export {};


Part of jest setup. we had it in index.d.ts before but now we declare it here. This will get autogenerated by typescript.

m2rads · 2025-01-02T01:51:45Z

shortest.config.ts

+  anthropicKey: process.env.ANTHROPIC_API_KEY || "",
  mailosaur: {
-    apiKey: process.env.MAILOSAUR_API_KEY,
-    serverId: process.env.MAILOSAUR_SERVER_ID,
+    apiKey: process.env.MAILOSAUR_API_KEY || "",
+    serverId: process.env.MAILOSAUR_SERVER_ID || "",


Have to do this otherwise typescript throws an error.

slavingia · 2025-01-02T02:27:49Z

If the expect changes are necessary, would be good to do in its own PR. cc @amk-dev

slavingia · 2025-01-02T02:37:46Z

Would be good to resolve merge conflicts now that the other branch was merged. I personally don't think doing || "" is a great idea, it pollutes the README/config and looks quite ugly. I'm sure there's a more elegant solution. Importing expect isn't a big deal, especially since we don't really need expect since the shortest itself should know what to look for to pass. So it'll act like a smell if it's included at all.

m2rads · 2025-01-02T02:48:53Z

I think for the env vars we can just have the user set them up in their .env file and we directly read them from there. I think Vercel AI SDK is like that.

For expect I had to update it in this PR otherwise it would have introduced a bug to the package. Since we already have expect implemented, if a user upgrade their shortest, their implementation would be broken.

If you don't think expect is necessary, I can delete the jestExpect implementation all together.

slavingia · 2025-01-02T02:50:35Z

Makes sense to me to remove it.

amk-dev · 2025-01-02T02:51:55Z

I personally don't think doing || "" is a great idea, it pollutes the README/config and looks quite ugly. I'm sure there's a more elegant solution.

the merged PR should take care of this.

Importing expect isn't a big deal, especially since we don't really need expect since the shortest itself should know what to look for to pass. So it'll act like a smell if it's included at all.

the merged PR preserves the current behaviour. expect is globally available. didn't want to introduce a breaking change.

I don't belive there's any functional issues remaining after that PR was merged. now its any new changes you guys want to make. (eg: remove expect)

one major difference from this PR to the merged PR is the use of tsup. its a pretty standared tool used on many popular projects. has a lot of features, but we're not using any atm apart from that global expect. but if you guys want to remove it, that's also fine.

amk-dev · 2025-01-02T03:00:38Z

I think for the env vars we can just have the user set them up in their .env file and we directly read them from there. I think Vercel AI SDK is like that.

had a discussion in the last PR about this.

#231 (comment)

after my PR merge,
-> any field that can accept an env variable is optional. the code has a fallback to the env variable.

m2rads · 2025-01-02T08:16:39Z

I think for the env vars we can just have the user set them up in their .env file and we directly read them from there. I think Vercel AI SDK is like that.

had a discussion in the last PR about this.

#231 (comment)

after my PR merge, -> any field that can accept an env variable is optional. the code has a fallback to the env variable.

Just saw your PR. This PR is not required anymore as your solves it in a better way.

m2rads added 5 commits January 1, 2025 17:40

refactor: auto generate index.d.ts by typescript and use import to us…

0ae9189

…e JestEexpect

update: removed index.d.ts and generate this using tsc

93173cb

conform to typescript errors

d264934

Update: import expect

8f6bcbd

update README to reflect the new changes

f6db69e

m2rads commented Jan 2, 2025

View reviewed changes

m2rads and others added 2 commits January 1, 2025 17:53

update test assertion file to use proper import for jestExpect

c7c20b0

[autofix.ci] apply automated fixes

8eeabb8

amk-dev mentioned this pull request Jan 2, 2025

chore: improve index.d.ts generation #231

Merged

vercel bot deployed to Preview January 2, 2025 01:55 View deployment

m2rads added 2 commits January 1, 2025 17:59

add autofix changes

291ffac

update pnpm-lock

37be246

vercel bot deployed to Preview January 2, 2025 02:01 View deployment

m2rads closed this Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto generate index.d.ts by typescript #238

Auto generate index.d.ts by typescript #238

m2rads commented Jan 2, 2025

vercel bot commented Jan 2, 2025 •

edited

Loading

m2rads left a comment

m2rads Jan 2, 2025

m2rads Jan 2, 2025

m2rads Jan 2, 2025

m2rads Jan 2, 2025

m2rads Jan 2, 2025

m2rads Jan 2, 2025

m2rads Jan 2, 2025

slavingia commented Jan 2, 2025

slavingia commented Jan 2, 2025

m2rads commented Jan 2, 2025

slavingia commented Jan 2, 2025

amk-dev commented Jan 2, 2025 •

edited

Loading

amk-dev commented Jan 2, 2025

m2rads commented Jan 2, 2025

Auto generate index.d.ts by typescript #238

Auto generate index.d.ts by typescript #238

Conversation

m2rads commented Jan 2, 2025

Changes

vercel bot commented Jan 2, 2025 • edited Loading

m2rads left a comment

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

m2rads Jan 2, 2025

Choose a reason for hiding this comment

slavingia commented Jan 2, 2025

slavingia commented Jan 2, 2025

m2rads commented Jan 2, 2025

slavingia commented Jan 2, 2025

amk-dev commented Jan 2, 2025 • edited Loading

amk-dev commented Jan 2, 2025

m2rads commented Jan 2, 2025

vercel bot commented Jan 2, 2025 •

edited

Loading

amk-dev commented Jan 2, 2025 •

edited

Loading