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

chore: remove unnecessary empty.d.ts #11966

Merged
merged 1 commit into from
Oct 16, 2021
Merged

chore: remove unnecessary empty.d.ts #11966

merged 1 commit into from
Oct 16, 2021

Conversation

mrazauskas
Copy link
Contributor

Summary

It looks like the empty.d.ts file in type tests does nothing. I think it can be removed safely just to have less noise around.

Here is why. By design tsd is running type checks (or test) on .d.ts type definition files. Not on .ts source, but on .d.ts inside the build artefact. Here in jest repo the type tests are set up exactly like this – the build files are tested.

The empty.d.ts is unofficial work around to run tests on .ts files. The thing is that tsd needs to have have some .d.ts to work. Install tsd in an empty folder and run yarn tsd. It will throw: "The type definition index.d.ts does not exist. Create one and try again." If an empty index.d.ts is added, tsd will complain that it can’t find any test.

Now the interesting part – add a test, but keep index.d.ts empty. Error: "./index.d.ts is not a module." That’s right! It has no exports, because we kept it empty. Now look at empty.d.ts file in this repo. No exports. Seems like this even isn’t valid .d.ts. Or?

After playing more with tsd I think that empty.d.ts is redundant in this repo. Might be I missed something. So let’s check what CI thinks.

In general this PR changes nothing. It is just a clean up with very long explanation. Thanks for reading (;

Test plan

After removal all works as expected locally. I was running type test before and after the change with Jest’s cache cleaned and the result is the same. Here I included failing test to avoid false positive. On CI nothing should fail.

Screenshot 2021-10-16 at 14 05 23

@codecov-commenter
Copy link

Codecov Report

Merging #11966 (05e0433) into main (7f39f0a) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11966      +/-   ##
==========================================
- Coverage   68.76%   68.76%   -0.01%     
==========================================
  Files         322      322              
  Lines       16621    16621              
  Branches     4797     4797              
==========================================
- Hits        11430    11429       -1     
- Misses       5158     5159       +1     
  Partials       33       33              
Impacted Files Coverage Δ
packages/expect/src/utils.ts 95.85% <0.00%> (-0.52%) ⬇️

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 7f39f0a...05e0433. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

nice!

@SimenB SimenB merged commit ed132a6 into jestjs:main Oct 16, 2021
@mrazauskas mrazauskas deleted the remove-empty-d-ts branch October 16, 2021 15:27
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants