Skip to content
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

Remove dependency on chai #20429

Merged
merged 2 commits into from
Dec 4, 2017
Merged

Remove dependency on chai #20429

merged 2 commits into from
Dec 4, 2017

Conversation

ghost
Copy link

@ghost ghost commented Dec 3, 2017

Removes the over 22000 line chai package and replaces it with under 50 lines of assertion functions.

(Background: Builds were failing because the latest @types/chai no longer creates a global variable for types. Normally this is a good thing but without #14844 we can no longer refer to the type in a global context.)

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I notice that the build still failed on this PR. Any idea why?

@ghost
Copy link
Author

ghost commented Dec 4, 2017

The tests succeed when run directly. At some point in the tests there's a global state change such that [] is no longer deeply equal to []. I'm looking into it.
(General global-state-in-tests tracking issue: #10683)

@mhegazy
Copy link
Contributor

mhegazy commented Dec 4, 2017

Should we update the @types/chai dependency to an older version for now?

@mhegazy
Copy link
Contributor

mhegazy commented Dec 4, 2017

At some point in the tests there's a global state change such that [] is no longer deeply equal to []

this is concerning.. I see you removed use of deepEqual from some tests, but what is the root cause?

@ghost
Copy link
Author

ghost commented Dec 4, 2017

I can use the previous commit as a repro but it will take a while to find out what went wrong. Can we merge this in the meantime?

@RyanCavanaugh RyanCavanaugh merged commit 66ec938 into master Dec 4, 2017
@ghost ghost deleted the unchai branch December 4, 2017 23:05
@ghost
Copy link
Author

ghost commented Dec 5, 2017

Fix for global state is at #20471

}

function isDeepEqual<T>(a: T, b: T): boolean {
if (a === b) {
Copy link
Member

@weswigham weswigham Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if we have any tests which could trigger this (probably not), but NaN === NaN is false, but I'd generally say one would expect { x: NaN } to deep equal { x: NaN }; ergo there should probably be a clause if (a !== a && b !== b) return true;. Probably a pointless remark unless we start testing more NaNy edgecases, though.

@weswigham
Copy link
Member

It's also worth noting that this change is going to give worse error output when debugging, since you'll no longer get pretty object diffs from any of the error messages from our custom isDeepEqual like chai would give.

weswigham added a commit to weswigham/TypeScript that referenced this pull request Dec 12, 2017
This reverts commit 66ec938, reversing
changes made to 37a4056.
ghost pushed a commit that referenced this pull request Dec 13, 2017
* Revert "Merge pull request #20429 from Microsoft/unchai"

This reverts commit 66ec938, reversing
changes made to 37a4056.

* Update lockfile
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants