feat: migrate to typescript #1567

43081j · 2023-12-30T20:07:51Z

This is an initial attempt at migrating the codebase to typescript.

The tests and build should pass but we can go further and do more with the types.

WIP until we do that.

cc @keithamus @koddsson

TODO

strengthen a lot of the types with inference etc
remove as many casts as possible
add some $ExpectType tests (there's an eslint plugin for this now)
run a formatter
add a linter

Some of these should probably be in their own PR.

For reviewers

Some things to review thoroughly:

the various overloads of assertion functions
- we do a lot of overloading which is super awkward in a strongly typed world, so these need a very close look
the addition of files in the package manifest
- make sure we pack the right files

43081j · 2023-12-30T20:14:35Z

package.json

+    "pathval": "^2.0.0",
+    "@types/check-error": "^1.0.3",
+    "@types/deep-eql": "^4.0.2",
+    "@types/pathval": "^1.1.2"


if you're curious: these are prod dependencies since we export some of them in our public interface. so typescript consumers need to pull these types down too

43081j · 2023-12-31T10:17:55Z

src/chai/assertion.ts

+  [k in keyof T]: T[k] extends (...args: never) => void ? k : never;
+}[keyof T];
+
+const getDefaultValue = (assertion: Assertion): Assertion => {


without this, we end up with circular imports: assertion.js -> addMethod.js -> assertion.js.

it is probably a bad idea to have such a circular dependency, so i extracted it into a factory function we now have to pass in (if we want an assertion to be the return value).

if anyone has a better idea, i'm all ears

43081j · 2023-12-31T10:18:48Z

src/chai/assertion.ts

+    util.flag(this, 'eql', config.deepEqual ?? util.eql);
+  }
+
+  public static create(


we can't really proxify ourselves, so i've introduced this new factory method.

you should call this near enough everywhere, instead of new Assertion

koddsson · 2024-01-05T13:10:02Z

WIP until we do that.

I'm gonna convert this to a draft while it's a WIP. Hope that's OK :)

koddsson · 2024-01-05T13:11:39Z

tsconfig.json

@@ -0,0 +1,19 @@
+{
+  "compilerOptions": {
+    "target": "es2021",


I wonder if we should figure out our support matrix and set this accordingly. Currently I'm not sure why we wouldn't change this to esnext or es2024.

you're right, i just chose it arbitrarily. i wasn't sure if esnext would be too loose and we should tie into a particular revision

Maybe we can set up a browserlists config to make sure we are adhering to whatever support matrix we decide.

Something like this?

https://browsersl.ist/#q=%22browserslist%22%3A+%5B%0A++%22defaults+and+fully+supports+es6-module%22%2C%0A++%22maintained+node+versions%22%0A%5D

SmashingQuasar · 2024-02-26T10:40:08Z

Hey @43081j !

I love Chai and I have been using it for years. This PR is extremely interesting and migrating to TypeScript would do a world of good to this library.
If you want, I'm ready to help you with this so we can lift Chai to the next level. Just let me know so we can sync' on this and work together to finish it up. :)

43081j · 2024-02-26T13:42:08Z

before we can really split the work up to contributors etc, i think i really need a thorough review from @koddsson or @keithamus to make sure we're on the right track and in agreement

there's a couple of awkward patterns lying around that i want to clean up before this can ever land:

passing a createDefaultValue around feels awkward, but its better than a circular import. i feel like there must be a third solution though with a bit of a refactor
logic has changed in some places, usually in functions with overloads. not sure how we want to deal with that in terms of semver
there's a bunch of casts i think we can get rid of with some thought

SmashingQuasar · 2024-02-26T14:58:26Z

I've browsed the code base quite a bit and I think there is some work to be done after migrating to TypeScript.
There are some code sections that look really difficult to understand and maintain. Especially those functions created within functions. There are also some wild var at different places and other things that look like they could be improved.

It's most likely an heirloom of some older sections of the code base which is to be expected. All of this to say that it would be easier to have a first base that is using TypeScript, event approximately, and then refactor fractions of the code base in different PRs.

My point is: I don't think it's too big of a deal if you have some things that aren't totally perfect, we can work on this afterwards. :)

I'm not a maintainer, not even a contributor yet, so it's just the opinion of one random dev'. ;)

This is an initial attempt at migrating the codebase to typescript. The tests and build should pass but we can go further and do more with the types. WIP until we do that.

This way, we can hold them in a non-generic array without the need for `any`. Later, when we need to call the method, we will have to cast it to a callable signature.

43081j · 2024-08-31T09:20:01Z

FYI i've caught this up and manually diffed against the current JS sources (since git won't do that for everything in this case)

everything seems fine so I think we should just ship a next tag or something and get people to try it out for a while

I considered the prev comment too but doing a typescript conversion a step at a time will be pretty difficult since everything is so intermingled

koddsson

This is great! I'm excited about having types, even if we can refine the types in the future I'm happy to start with something like this and then add more type coverage as we go along.

Maybe we can create a next branch which will automatically publish to the next tag on npm and we can see if we can work together to make small PRs towards that branch until we're satisfied with the functionality and type-coverage. At that point we can make a PR main and get that published as chai@6.

Only change I would like to make before continuing is to make sure that files we are renaming/moving retains their history and don't show up as deleted and recreated. Currently in this PR the files lib/chai/assertion.js, lib/chai/interface/expect.js and lib/chai/utils/flag.js are deleted when they should be moved so that they retain the history.

koddsson · 2024-10-10T10:00:22Z

package.json

@@ -29,14 +34,15 @@
  "main": "./chai.js",
  "scripts": {
    "prebuild": "npm run clean",
-    "build": "npm run build:esm",
-    "build:esm": "esbuild --bundle --format=esm --keep-names --outfile=chai.js index.js",
+    "build": "npm run build:ts && npm run build:esm",


Why are we doing both? Is it to check for type errors? If yes, can we create a "lint:types": "tsc --noEmit" task and run that in the existing lint task?

koddsson · 2024-10-10T10:04:15Z

package.json

@@ -56,6 +65,7 @@
    "esbuild": "^0.19.10",
    "eslint": "^8.56.0",
    "eslint-plugin-jsdoc": "^48.0.4",
-    "mocha": "^10.2.0"
+    "mocha": "^10.2.0",
+    "typescript": "^5.3.3"


What do you think about changing this to a tilde since TypeScript doesn't follow semver?

Suggested change

"typescript": "^5.3.3"

"typescript": "~5.3.3"

microsoft/TypeScript#14116

koddsson · 2024-10-10T10:05:50Z

src/chai/assertion.ts

Can you change this to a rename/move so that we keep the git history of this file intact?

they're all renames, git just can't figure it out. don't think there's much we can do here

locally, I've been using git diff main:foo.js foo.ts or something like that

afaik git determines if its a move at the time of diff, not in commit metadata. so its probably just too big of a change for it to infer

Did you move the files with git mv or just move them normally? If I recall git mv will do the right thing but moving normally, git can't recognize if the file is moved or not if the delta between the changes is high enough.

I moved them with git mv

Sometimes git just doesn't manage to keep track of them. I've seen this many times in big refactors but not sure why

Weird! But yeah no worries then.

koddsson · 2024-10-10T10:06:00Z

src/chai/interface/expect.ts

Can you change this to a rename/move so that we keep the git history of this file intact?

koddsson · 2024-10-10T10:09:53Z

src/chai/utils/addChainableMethod.ts

+/*!
+ * Module dependencies
+ */


/*! comments are generally considered "legal comments" and will not be removed by bundlers. Probably users will not be bundling this but I still think this could be a normal comment.

Suggested change

/*!

* Module dependencies

*/

/**

* Module dependencies

*/

I'm also thinking we could just remove it as it's clear that these are dependencies.

Suggested change

/*!

* Module dependencies

*/

koddsson · 2024-10-10T10:11:21Z

src/chai/utils/flag.ts

Can you change this to a rename/move so that we keep the git history of this file intact?

koddsson · 2024-10-10T10:17:38Z

(Also sorry for the really late review, I've been moving countries and I haven't been checking my notifications enough. Always feel free to ping (and re-ping) me for reviews <3)

43081j requested a review from a team as a code owner December 30, 2023 20:07

43081j force-pushed the typescript branch from eaa2057 to 97ffc58 Compare December 30, 2023 20:11

43081j commented Dec 30, 2023

View reviewed changes

43081j force-pushed the typescript branch from 97ffc58 to 721985c Compare December 31, 2023 10:15

43081j commented Dec 31, 2023

View reviewed changes

koddsson marked this pull request as draft January 5, 2024 13:10

koddsson reviewed Jan 5, 2024

View reviewed changes

koddsson mentioned this pull request Jan 24, 2024

Add types #1557

Closed

sheremet-va mentioned this pull request Feb 7, 2024

fix!: update dependency chai to v5 vitest-dev/vitest#5135

Merged

1 task

43081j force-pushed the typescript branch from 36cbcf8 to 3923df8 Compare February 25, 2024 14:37

43081j force-pushed the typescript branch from 46c5f7b to 6a1b5e9 Compare May 4, 2024 17:24

43081j added 11 commits August 31, 2024 15:45

feat: migrate to typescript

ccd7706

This is an initial attempt at migrating the codebase to typescript. The tests and build should pass but we can go further and do more with the types. WIP until we do that.

fix: use never for method args

4beb6f5

This way, we can hold them in a non-generic array without the need for `any`. Later, when we need to call the method, we will have to cast it to a callable signature.

feat: make assertion type generic

7e614bd

feat: add a crazy amount of types

5c0e656

feat: strongly typed property assertions

c8c24a1

fix: clean up change types

9b98008

feat: strongly type keys of change assertions

133e05c

feat: strong assertions of properties

50ded4d

feat: strongly typed typeof hints

e175899

feat: add a few negated assertions

941cbe8

feat: assert iterable types

2e165b2

43081j force-pushed the typescript branch from 6a1b5e9 to 2e165b2 Compare August 31, 2024 08:45

chore: reset package lock

aedefa2

koddsson reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate to typescript #1567

feat: migrate to typescript #1567

43081j commented Dec 30, 2023

43081j Dec 30, 2023

43081j Dec 31, 2023

43081j Dec 31, 2023

koddsson commented Jan 5, 2024

koddsson Jan 5, 2024

43081j Jan 5, 2024

koddsson Jan 5, 2024

koddsson Jan 5, 2024

SmashingQuasar commented Feb 26, 2024

43081j commented Feb 26, 2024

SmashingQuasar commented Feb 26, 2024

43081j commented Aug 31, 2024

koddsson left a comment

koddsson Oct 10, 2024

koddsson Oct 10, 2024

koddsson Oct 10, 2024

43081j Oct 10, 2024

koddsson Oct 10, 2024

43081j Oct 11, 2024 •

edited

Loading

koddsson Oct 11, 2024

koddsson Oct 10, 2024

koddsson Oct 10, 2024

koddsson Oct 10, 2024

koddsson commented Oct 10, 2024 •

edited

Loading

feat: migrate to typescript #1567

Are you sure you want to change the base?

feat: migrate to typescript #1567

Conversation

43081j commented Dec 30, 2023

TODO

For reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koddsson commented Jan 5, 2024

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

SmashingQuasar commented Feb 26, 2024

43081j commented Feb 26, 2024

SmashingQuasar commented Feb 26, 2024

43081j commented Aug 31, 2024

koddsson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

43081j Oct 11, 2024 • 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

koddsson commented Oct 10, 2024 • edited Loading

43081j Oct 11, 2024 •

edited

Loading

koddsson commented Oct 10, 2024 •

edited

Loading