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

Update types + add TS tests #77

Merged
merged 20 commits into from
May 21, 2020
Merged

Update types + add TS tests #77

merged 20 commits into from
May 21, 2020

Conversation

kale-stew
Copy link
Contributor

@kale-stew kale-stew commented May 12, 2020

Description

This PR
(1) introduces a file to more thoroughly test our type definitions
(2) introduces docs surrounding these tests
(3) update our TS guidelines
(4) Fixes our type defs where needed

Still running into an issue with timeout in the Preact beforeEach

Closes #69

Checklist:

  • All tests are passing
  • Benchmark performance has not significantly decreased (tests won't affect benchmarks)
  • Bundle size has not been significantly impacted (^)
  • The bundle size badge has been updated to reflect the new size (won't be necessary)

CONTRIBUTING.md Outdated Show resolved Hide resolved
@kale-stew kale-stew force-pushed the types/docs-and-testing branch 2 times, most recently from 5c3c068 to acc9ea3 Compare May 13, 2020 15:51
package.json Outdated Show resolved Hide resolved
test/typescript/index.ts Outdated Show resolved Hide resolved
test/typescript/index.ts Outdated Show resolved Hide resolved
@kale-stew kale-stew changed the title [WIP] Update types + add TS tests Update types + add TS tests May 20, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #77 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #77   +/-   ##
=======================================
  Coverage   98.21%   98.21%           
=======================================
  Files           1        1           
  Lines          56       56           
=======================================
  Hits           55       55           
  Misses          1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf6d0c2...3dfb3cc. Read the comment docs.

Copy link
Contributor

@chrisbolin chrisbolin left a comment

Choose a reason for hiding this comment

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

just a few things. should all be pretty minor

CONTRIBUTING.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/typescript/index.js Outdated Show resolved Hide resolved
test/typescript/index.tsx Outdated Show resolved Hide resolved
test/typescript/index.tsx Outdated Show resolved Hide resolved
test/typescript/index.tsx Outdated Show resolved Hide resolved
@chrisbolin
Copy link
Contributor

NOTE: as is this will not close #61, so we should make sure that doesn't happen. A test for #61 can be added in a new PR (this one is big enough :) )

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@ryan-roemer ryan-roemer left a comment

Choose a reason for hiding this comment

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

Just some cleanup comments after which LGTM!

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ryan-roemer
Copy link
Member

Lint failures in appveyor:


24C:\projects\react-fast-compare\test\typescript\sample-usage.tsx
25  23:7  error  'TestChild' is defined but never used      no-unused-vars
26  34:7  error  'TestContainer' is defined but never used  no-unused-vars

@ryan-roemer
Copy link
Member

I think a simple export of test container will fix the error. Bigger question is why Travis didn’t flag the failure

@kale-stew
Copy link
Contributor Author

We intentionally pulled tslint out of the test script yesterday because we didn't want to muck around in lint configs for our test files for too long, but this morning the issue has been identified as a react-specific lint plugin which I've now introduced. tslint is back into our CI script, and this push ^ should fix things

@kale-stew kale-stew merged commit e01e975 into master May 21, 2020
@kale-stew kale-stew deleted the types/docs-and-testing branch May 21, 2020 15:08
@chrisbolin
Copy link
Contributor

nice work! and thanks for creating #79 so we can return to the linting question later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Types: documentation, clean up, and testing
4 participants