feat: migrate some files to typescript #848

marcosvega91 · 2020-12-10T17:12:47Z

follow-up #614

I have started migrating @testing-library/dom to typescript. The idea is also to complete step by step PR #834

Types are not generated until everything is completed

Any suggestions and advice are always welcome 🙏🏽

I want to add more files to this PR.

What: File migrated

config.js
matches.js
label-helpers.js

Why: Is cool and fun

How:

Checklist:

Documentation added to the
docs site
Tests
Typescript definitions updated
Ready to be merged

codesandbox-ci · 2020-12-10T17:16:17Z

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 d15238b:

Sandbox	Source
react-testing-library-examples	Configuration

codecov · 2020-12-10T17:16:20Z

Codecov Report

Merging #848 (d15238b) into master (f753c8a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #848   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          941       945    +4     
  Branches       289       290    +1     
=========================================
+ Hits           941       945    +4

Impacted Files	Coverage Δ
src/events.js	`100.00% <ø> (ø)`
src/config.ts	`100.00% <100.00%> (ø)`
src/label-helpers.ts	`100.00% <100.00%> (ø)`
src/matches.ts	`100.00% <100.00%> (ø)`

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 f753c8a...d15238b. Read the comment docs.

kentcdodds

Very cool @marcosvega91 👏

kentcdodds · 2020-12-10T17:58:00Z

src/__tests__/config.js

@@ -1,4 +1,4 @@
-import {configure, getConfig} from '../config'
+import {configure, getConfig} from '../config.ts'


Interesting. Is the extension required? I didn't think it was and would prefer to not have to specify it.

I don't like too, but I didn't find a way to not specify it. eslint gave me some trouble.

We can deal with that later. Not a ship stopper.

I'll leave the point open, maybe someone could help

src/config.ts

kentcdodds

We need to remember that the types directory should be completely removed when this migration is finished. There's no reason to keep it around once we can generate the types from the source.

But for now, this is perfect 👍

kentcdodds · 2020-12-10T18:30:54Z

Personally, I think let's get this merged, then we can continue to iterate and avoid large branches.

marcosvega91 · 2020-12-10T18:42:39Z

I have only one more file

marcosvega91 · 2020-12-10T18:50:07Z

ready for now

kentcdodds

Looking good to me. I don't plan to merge this myself, but I did leave a few thoughts and ideas that aren't a big deal either way.

src/label-helpers.ts

kentcdodds · 2020-12-10T19:01:51Z

src/label-helpers.ts

+  } else if ('value' in node) {
+    textContent = node.value


Suggested change

} else if ('value' in node) {

textContent = node.value

kentcdodds · 2020-12-10T19:02:19Z

src/label-helpers.ts

  } else {
-    textContent = node.value || node.textContent
+    textContent = node.textContent


Could this work?

Suggested change

textContent = node.textContent

textContent = node?.value ?? node.textContent

Or even

Suggested change

textContent = node.textContent

textContent = node.value ?? node.textContent

The problem here is that node can't be null but at the same time only input has value.
So doing node?.value is not correct because node is not nulla and node.value is not correct because input doesn't have a value

src/config.ts

src/label-helpers.ts

eps1lon · 2020-12-10T21:45:50Z

src/label-helpers.ts

-function getRealLabels(element) {
-  if (element.labels !== undefined) return element.labels ?? []
+function getRealLabels(element: Element | HTMLInputElement) {
+  // for old browsers
+  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+  if ('labels' in element && element.labels !== undefined)
+    return element.labels ?? []


I would prefer we don't change any runtime behavior but rather override the typechecker with type-casting or @ts-expect-error. Right now I can't know from the review whether this changes runtime behavior, or was unsound, or just TS being overly strict.

The problem here is the labels can be null and not undefined. Btw I think that on old browser like ie labels is undefined and not null

Looks to me covered all these cases and TypeScript is not able to narrow. Let's keep the runtime and annotate the compile time properly.

The problem is that the linter is giving the problem, so @ts-expect-error will not work

Ignore the linter.

src/label-helpers.ts

src/utils.ts

tests/jest.config.dom.js

types/matches.d.ts

types/utils.d.ts

eps1lon

Proposed follow-up in marcosvega91#2

Should have the minimal set of runtime changes. Kept the switch from match to test which could be merged regardless of types.

commit 6563b96 Author: eps1lon <silbermann.sebastian@gmail.com> Date: Fri Dec 11 14:33:36 2020 +0100 Remove .ts extensions commit 6c08f92 Author: eps1lon <silbermann.sebastian@gmail.com> Date: Fri Dec 11 14:17:30 2020 +0100 Polish - revert most runtime changes - test over exec because semantics

eps1lon · 2020-12-11T13:56:31Z

Forgot to update the PR title of the recently merged #821. Changing this PR to a feature (can always do that) so that we can kick of a release.

marcosvega91 · 2020-12-11T13:57:50Z

thanks for the help @eps1lon :)

eps1lon · 2020-12-11T14:05:10Z

package.json

-    "extends": "./node_modules/kcd-scripts/eslint.js",
+    "extends": [
+      "./node_modules/kcd-scripts/eslint.js",
+      "plugin:import/typescript"
+    ],


As recommended: https://github.com/benmosher/eslint-plugin-import#typescript

Yes I see, the strange thing is that is should already be in kcd

It should be. That's an oversight

github-actions · 2020-12-11T14:23:38Z

🎉 This PR is included in version 7.29.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

marcosvega91 added 3 commits December 10, 2020 12:44

feat: enable typescript and disable type generation

f215937

chore: update kcd-script and skip types generation

61214fc

chore: extend types tsconfig from parent

b71dccd

marcosvega91 added 2 commits December 10, 2020 18:53

chore: convert config.js to typescript

c3b9704

chore: migrate matches.js to typescript

8c82991

marcosvega91 force-pushed the pr/pr_typescript_migration branch from d3ce72c to 8c82991 Compare December 10, 2020 17:54

kentcdodds reviewed Dec 10, 2020

View reviewed changes

chore: use types from old folder

2da06c8

kentcdodds previously approved these changes Dec 10, 2020

View reviewed changes

chore: migrate label-helpers to typescript

9942434

marcosvega91 dismissed kentcdodds’s stale review via 9942434 December 10, 2020 18:49

kentcdodds previously approved these changes Dec 10, 2020

View reviewed changes

eps1lon requested changes Dec 10, 2020

View reviewed changes

fix: apply review suggestions

03f20e1

marcosvega91 dismissed kentcdodds’s stale review via 03f20e1 December 11, 2020 08:45

eps1lon mentioned this pull request Dec 11, 2020

Polish types marcosvega91/dom-testing-library#2

Closed

eps1lon previously approved these changes Dec 11, 2020

View reviewed changes

eps1lon changed the title ~~chore: migrate some files to typescript~~ feat migrate some files to typescript Dec 11, 2020

eps1lon changed the title ~~feat migrate some files to typescript~~ feat: migrate some files to typescript Dec 11, 2020

eps1lon dismissed their stale review via a2b9e44 December 11, 2020 13:55

eps1lon reviewed Dec 11, 2020

View reviewed changes

Merge branch 'master' into pr/pr_typescript_migration

d15238b

eps1lon merged commit accb6cc into testing-library:master Dec 11, 2020

github-actions bot added the released label Dec 11, 2020

This was referenced Dec 17, 2020

feat: migrate suggestions/pretty-dom/helpers/role-helpers to ts #850

Open

fix: add number support for matches #856

Merged

nathanforce mentioned this pull request Jan 3, 2021

fix: Pass container from findBy through to waitFor #868

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate some files to typescript #848

feat: migrate some files to typescript #848

marcosvega91 commented Dec 10, 2020 •

edited

Loading

codesandbox-ci bot commented Dec 10, 2020 •

edited

Loading

codecov bot commented Dec 10, 2020 •

edited

Loading

kentcdodds left a comment

kentcdodds Dec 10, 2020

marcosvega91 Dec 10, 2020

kentcdodds Dec 10, 2020

marcosvega91 Dec 10, 2020

kentcdodds left a comment

kentcdodds commented Dec 10, 2020

marcosvega91 commented Dec 10, 2020

marcosvega91 commented Dec 10, 2020

kentcdodds left a comment

kentcdodds Dec 10, 2020

kentcdodds Dec 10, 2020

marcosvega91 Dec 11, 2020

eps1lon Dec 10, 2020

marcosvega91 Dec 10, 2020

eps1lon Dec 11, 2020

marcosvega91 Dec 11, 2020

eps1lon Dec 11, 2020

eps1lon left a comment

eps1lon commented Dec 11, 2020

marcosvega91 commented Dec 11, 2020

eps1lon Dec 11, 2020

marcosvega91 Dec 11, 2020 •

edited

Loading

kentcdodds Dec 11, 2020

github-actions bot commented Dec 11, 2020

		@@ -1,4 +1,4 @@
		import {configure, getConfig} from '../config'
		import {configure, getConfig} from '../config.ts'

	textContent = node.textContent
	textContent = node?.value ?? node.textContent

	textContent = node.textContent
	textContent = node.value ?? node.textContent

feat: migrate some files to typescript #848

feat: migrate some files to typescript #848

Conversation

marcosvega91 commented Dec 10, 2020 • edited Loading

codesandbox-ci bot commented Dec 10, 2020 • edited Loading

codecov bot commented Dec 10, 2020 • edited Loading

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Dec 10, 2020

marcosvega91 commented Dec 10, 2020

marcosvega91 commented Dec 10, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon commented Dec 11, 2020

marcosvega91 commented Dec 11, 2020

Choose a reason for hiding this comment

marcosvega91 Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2020

marcosvega91 commented Dec 10, 2020 •

edited

Loading

codesandbox-ci bot commented Dec 10, 2020 •

edited

Loading

codecov bot commented Dec 10, 2020 •

edited

Loading

marcosvega91 Dec 11, 2020 •

edited

Loading