-
-
Notifications
You must be signed in to change notification settings - Fork 694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: migrate to typescript #1567
base: main
Are you sure you want to change the base?
Conversation
"pathval": "^2.0.0", | ||
"@types/check-error": "^1.0.3", | ||
"@types/deep-eql": "^4.0.2", | ||
"@types/pathval": "^1.1.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/chai/assertion.ts
Outdated
[k in keyof T]: T[k] extends (...args: never) => void ? k : never; | ||
}[keyof T]; | ||
|
||
const getDefaultValue = (assertion: Assertion): Assertion => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/chai/assertion.ts
Outdated
util.flag(this, 'eql', config.deepEqual ?? util.eql); | ||
} | ||
|
||
public static create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I'm gonna convert this to a draft while it's a WIP. Hope that's OK :) |
@@ -0,0 +1,19 @@ | |||
{ | |||
"compilerOptions": { | |||
"target": "es2021", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can set up a browserlists
config to make sure we are adhering to whatever support matrix we decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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:
|
I've browsed the code base quite a bit and I think there is some work to be done after migrating to TypeScript. 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.
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
$ExpectType
tests (there's an eslint plugin for this now)Some of these should probably be in their own PR.
For reviewers
Some things to review thoroughly:
files
in the package manifest