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

Copied types from DefinitelyTyped into this repository #530

Merged
merged 12 commits into from
May 7, 2020

Conversation

Lagily
Copy link
Contributor

@Lagily Lagily commented Apr 22, 2020

What: Copied types from DefinitelyTyped into this repository and adjusted the package.json, so these types are packed into the npm package. Also removed the dependency to the DefinitelyTyped types, since they are now in this repo.

Why: See #494 (comment)

Checklist:

  • [?] Documentation added to the
    docs site
  • [N/A] I've prepared a PR for types targeting
    DefinitelyTyped
  • Tests
  • Ready to be merged

Not sure as to where and how you would like documentation to be added. I don't think additional documentation is needed (yet). We should instead make a PR to remove dom-testing-library from DefinitelyTyped - what do you think?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 22, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 430236e:

Sandbox Source
festive-bash-udo11 Configuration

@codecov
Copy link

codecov bot commented Apr 22, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #530   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          438       438           
  Branches       105       105           
=========================================
  Hits           438       438           

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 9d62141...430236e. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for this!

Is there any way we can add something to the validate script that will check these typings are correct? I've had some projects where they'll have a set of TypeScript files in the project and run the typings against those files to make sure they pass type checking.

@eps1lon
Copy link
Member

eps1lon commented Apr 22, 2020

@kentcdodds Would you be open to adding type annotations via JSDOC?

We could generate the declarations directly from the source files while simultaneously checking that the types are correct (and actually type check the source files).

That way we keep any TypeScript syntax out and the current workflow doesn't change.

At least I think this is possible. The typings are simple enough that JSDOC should be sufficient.

@kentcdodds
Copy link
Member

I think that would be a good iterative approach to migrating to TypeScript (if that's what we end up doing). I'd be willing to take a look at that, yeah.

@Lagily
Copy link
Contributor Author

Lagily commented Apr 23, 2020

Sure, I think DefinitelyTyped also runs some sort of tests on their types, I'll investigate and add something

@Lagily
Copy link
Contributor Author

Lagily commented Apr 23, 2020

I've added the type tests like DefinitelyTyped does it, I commited in a way that you can see the tests fail, before adjustments are made. If you change the type-tests.ts in a way that types don't make sense (as in it doesn't compile anymore) the npm verify step will now fail.

I honestly don't think it's too much work to port this code base to TypeScript, I personally wouldn't add any more intermediate steps.

Copy link
Member

@eps1lon eps1lon 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 to me with one minor nit.

Just need to decide in which version we want to drop this.

The jsdoc + js-check effort can be made in another PR with the goal of keeping the types in sync.

package.json Outdated Show resolved Hide resolved
@Lagily
Copy link
Contributor Author

Lagily commented Apr 24, 2020

I might give it a shot to port the code base to TypeScript - should I wait out on that?

@eps1lon
Copy link
Member

eps1lon commented Apr 24, 2020

I might give it a shot to port the code base to TypeScript - should I wait out on that?

For now we don't want to use actual TypeScript. After we resolved this PR I'm going to try out types via JSDOC.

@@ -0,0 +1,8 @@
export function waitForElementToBeRemoved<T>(
Copy link
Member

Choose a reason for hiding this comment

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

Fyi, I made a PR to DefinitelyTyped to add the interval option here.
It got merged today, so I believe we should also add it here.

https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44006/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually since options is simply passed to waitFor, it should just use the same type, so we don't have to redefine it in multiple places, I'll take care of it

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea 👍

@eps1lon
Copy link
Member

eps1lon commented Apr 30, 2020

Regarding JSDOC: It doesn't seem like we can incorporate TS types at the moment. I wouldn't know how to type events.js.

@kentcdodds
Copy link
Member

Ok, I'm thinking this is a good idea. Shame about the JSDOC stuff. Let's go ahead and get this merged and then we can explore migrating to TS totally.

Looks like there's a conflict in the package.json (sorry, my bad). I just released a major version to kcd-scripts which now will automatically run a typecheck script if it's available. So you don't need to worry about adding that to the validate script.

@kentcdodds kentcdodds closed this May 5, 2020
@kentcdodds kentcdodds reopened this May 5, 2020
@kentcdodds
Copy link
Member

Sorry, clicked the wrong button.

Also sorry about that, I mistakenly removed jsdom from the dev deps even though I forgot we actually use it directly for our node tests.

Go ahead and rebase again if you please and make sure to re-install deps. Thanks!

@Lagily
Copy link
Contributor Author

Lagily commented May 5, 2020

Sure! Will hopefully run through now - does merging this PR count as a 'migrate to TS pull request is welcome'? :-D

@kentcdodds
Copy link
Member

A pull request would be welcome, but I'm not certain that it would be merged.

To be frank, I'm insecure about my personal TypeScript abilities (just haven't had enough time with it honestly) so I've been hesitant to add TS to my projects primarily for that reason (though the barrier to contributions is also a consideration).

So I'm willing to take a look and provide feedback on a PR, but I'm not 100% certain that it would get merged.

@kentcdodds
Copy link
Member

Is there anything left on this PR?

I want to leave this open for a day or two more to make sure everyone who's got an opinion shares it.

Merging this PR means that every API change should be associated with updates to the TypeScript definitions, so it has important implications.

@Lagily
Copy link
Contributor Author

Lagily commented May 5, 2020

Fair enough. From my point of view, this PR is complete.

@kentcdodds
Copy link
Member

Ok, let's do this.

@kentcdodds kentcdodds merged commit 678f4da into testing-library:master May 7, 2020
@kentcdodds
Copy link
Member

🎉 This PR is included in version 7.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nstepien
Copy link
Contributor

nstepien commented May 7, 2020

I use @testing-library/react and I'm getting these errors now:

> tsc -p tsconfig.all.json

node_modules/@testing-library/dom/types/queries.d.ts:3:32 - error TS2307: Cannot find module 'wait-for'.

3 import { waitForOptions } from 'wait-for';
                                 ~~~~~~~~~~

node_modules/@testing-library/dom/types/wait-for-dom-change.d.ts:1:32 - error TS2307: Cannot find module 'index'.

1 import { waitForOptions } from "index";
                                 ~~~~~~~

node_modules/@testing-library/dom/types/wait-for-element-to-be-removed.d.ts:1:32 - error 
TS2307: Cannot find module 'wait-for'.

1 import { waitForOptions } from "wait-for";
                                 ~~~~~~~~~~

node_modules/@testing-library/dom/types/wait-for-element.d.ts:1:32 - error TS2307: Cannot find module 'wait-for'.

1 import { waitForOptions } from "wait-for";
                                 ~~~~~~~~~~


Found 4 errors.

Looks like you need to use relative paths.

@nstepien
Copy link
Contributor

nstepien commented May 7, 2020

@kentcdodds I've opened a PR to try to fix the above: #556

@Lagily
Copy link
Contributor Author

Lagily commented May 16, 2020

@kentcdodds could you also add me as a contributor?

@eps1lon
Copy link
Member

eps1lon commented May 16, 2020

@all-contributors add @Lagily for code

@allcontributors
Copy link
Contributor

@eps1lon

I've put up a pull request to add @Lagily! 🎉

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

Successfully merging this pull request may close these issues.

5 participants