Proposal setup to make the TypeScript migration easier #614

ghostd · 2020-06-09T15:00:52Z

What:

Add a TypeScript config to generate the declaration files from the TS source files.

Why:

The aim is to make the TS transition easier (and avoid big PR as mentioned here)

How:
I added a "manual" script generate-types which generates the declaration files from the TS source files. This approach allows us to migrate each JS file one by one. I don't know what could be the best way to insert this command into the build/validation process.

This PR includes a first migration (see get-node-text.ts)

Checklist:

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

Any thoughts are welcome.

#494

codesandbox-ci · 2020-06-09T15:02:00Z

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 3e8c451:

Sandbox	Source
kentcdodds/react-testing-library-examples	Configuration

ghostd · 2020-06-09T15:54:00Z

I'm not sure to understand why the build fails. The "typecheck" step works for me :-/

kentcdodds

Maybe the reason it's working on your machine and not on CI is a version mis-match. I think fixing the error should be pretty straightforward though.

This is looking pretty good. Could you try and move one other file to TypeScript so we get a more realistic view of what this would be like?

babel.config.js

Disable some tslint rules which conflicts with the declarations generated by tsc Also remove the useless babel.config.js

kentcdodds

Feeling pretty comfortable with this generally. I'm open to seeing what this looks like if the repo is 100% TypeScript.

kentcdodds · 2020-06-09T20:47:52Z

types/event-map.d.ts

@@ -0,0 +1,72 @@
+declare type EventMapKey = string


Are .d.ts files necessary when the source is written in TS?

Shouldn't be

When the migration will be over, the types directory can be removed from the source; the declaration files will be generated during the build phase.

Can we not start generating the types before everything is migrated?

Not sure. tsc uses the typescript source files to extract the declarations. i have no idea how to do that before everything is migrated. If someone knows a way i'd be very glad to try it.

I'm not 100% sure I understand what you're wanting to do but tsc only runs on typescript files so if you want it to generate typescript definition files for only those that have been moved over that should happen naturally. I'm sure we could find what you're looking for in the compiler docs. But if you clarify what you are looking to do I could do more research.

tests/jest.config.dom.js

tests/jest.config.node.js

kentcdodds · 2020-06-09T20:51:57Z

types/get-node-text.d.ts

@@ -1 +1,2 @@
-export function getNodeText(node: HTMLElement): string;
+declare function getNodeText(node: HTMLElement): string;
+export { getNodeText };


Same here. Do we need .d.ts files when the source is in TypeScript?

shouldn't need to. Might need to add the -d flag when compiling or set that in your tsconfig.

It seems the prettier settings can conflit with the 'whitespace' rule of dtslint. Also forces the dtslint version to be sure Travis use a version with this fix: microsoft/dtslint#295

types/event-map.d.ts

codecov · 2020-06-10T19:09:19Z

Codecov Report

Merging #614 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #614   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          555       559    +4     
  Branches       138       141    +3     
=========================================
+ Hits           555       559    +4

Impacted Files	Coverage Δ
src/event-map.ts	`100.00% <100.00%> (ø)`
src/get-node-text.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 281eb8b...3e8c451. Read the comment docs.

kentcdodds

This is great. I'm happy to merge this as-is and then we can work on converting the rest over. But I'd like another 👍 from someone before I do.

smeijer · 2020-06-11T09:37:49Z

types/events.d.ts

@@ -1,95 +1,104 @@
 export type EventType =


Can't we use the native HTMLElementEventMap or WindowEventMap instead of maintaining a custom type?

Didn't know this existed. I'd prefer this 👍

Yeah, addEventListener is typed, so my first thought is that we should be able to reuse those types.

// HTMLDocument addEventListener<K extends keyof DocumentEventMap>(type: K, listener: (this: Document, ev: DocumentEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void; addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void; // HTMLElement addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLElement, ev: HTMLElementEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void; addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;

@ghostd, please correct me if I'm wrong. I might miss something fundamental.

From my understanding, removing the custom EventType will not prevent us to maintain the event map (since the metadata are 'erased' at runtime), and for now, the lib.dom.ts is still missing some events (composition ones and maybe others).
Maybe we can have a custom type which would be a union of the DocumentEventMap, HTMLElementEventMap and the missing events used by dom-testing-library. In this way we will just maintain the missing types. I'll give a shot

I think i misunderstood your point.

We can simply define EventType this way:

type EventType = keyof (typeof eventMap);

When we add an entry into eventMap, TS will infer the new definition of EventType

Does this answer to your point?

I think I would prefer this solution.

@eps1lon, what do you think?

Sounds good to me. You two seem to know more about built-in types for this problem so I can't help much.

I found this as an example from the documentation:

fireEvent.keyDown(domNode, { key: 'Enter', code: 'Enter' })

The case of the keys of eventMap seems to be part of the public API. Changing this would be a breaking change. I propose to have keydown in eventMap (so the key will be type checked against the lib.dom.ts interface), and add keyDown into eventAliasMap

I'll do a commit this day to show the changes.

I've started something, and now i've to maintain the camelcase event list into the eventAliasMap:

export const eventAliasMap: Record<string, EventType> = { doubleClick: 'dblclick', // Aliases to keep the API with camelcase events // Composition Events compositionEnd: 'compositionend', compositionStart: 'compositionstart', compositionUpdate: 'compositionupdate', // Keyboard Events keyDown: 'keydown', keyPress: 'keypress', keyUp: 'keyup', // Focus Events focusIn: 'focusin', focusOut: 'focusout', // Mouse Events contextMenu: 'contextmenu', dblClick: 'dblclick', dragEnd: 'dragend', dragEnter: 'dragenter', dragExit: 'dragexit', dragLeave: 'dragleave', dragOver: 'dragover', dragStart: 'dragstart', mouseDown: 'mousedown', mouseEnter: 'mouseenter', mouseLeave: 'mouseleave', mouseMove: 'mousemove', mouseOut: 'mouseout', mouseOver: 'mouseover', // ...more and more... }

We will to need an AliasEventType to correctly declare the all the fireEvent aliases (fireEvent.dblClick, fireEvent.mouseEnter, and so on). It seems we just shift the problem a little further.

The library API exposes camelcase events, which is more usable/readable for the end user IMHO. Maybe we should wait for the end of the TS migration before trying to simplify the code. For now, TS should be a "helper", not a constraint to build a clean API.

If you really want to see all the modifications i can spend more time on it this weekend. Let me know your opinions

If you really want to see all the modifications i can spend more time on it this weekend

Nah, let's not waste your time. I agree with:

Maybe we should wait for the end of the TS migration before trying to simplify the code.

We've (you've) explored the available options. It looks like we have the best one for this moment.

So let's get this thing merged 👍

smeijer · 2020-06-11T09:40:45Z

types/event-map.d.ts

+} & KeyboardEventInit
+declare type EventMapValue =
+  | {
+      EventType: 'AnimationEvent'


Do we want EventType here to be a string, or the type it represents from lib.dom?

EventType: 'AnimationEvent' | AnimationEvent

Not sure which I favor. I'm just curious about the reason why (pro/cons).

The string is used at runtime:

dom-testing-library/src/events.js

Lines 51 to 58 in 281eb8b

const EventConstructor = window[EventType] || window.Event

let event

/* istanbul ignore else */

if (typeof EventConstructor === 'function') {

event = new EventConstructor(eventName, eventInit)

} else {

// IE11 polyfill from https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent#Polyfill

event = window.document.createEvent(EventType)

So we need a TS macro to transform a TS interface name into string. I don't know how to do that.

kentcdodds · 2020-06-13T14:14:07Z

I think we'll want to put a hold on this until #616 is merged (which should be early next week).

nickserv · 2020-06-13T23:58:48Z

Types can often help with big refactors/additions like this, there's often some extra churn involved but I usually feel it's worth it. Do you have any concerns about if merging this first would cause issues, or is it just that you'd prefer to prioritize the merge first?

kentcdodds · 2020-06-14T00:06:52Z

It's just because I think we may not be quite ready to merge this. And I really want to prioritize merging in user event.

nickserv · 2020-06-14T00:07:49Z

Thanks I'm fine with that, I just wasn't sure about the status

nickserv · 2020-06-16T12:11:19Z

FYI: The user-event merge has been cancelled. We should be ready to work on this again if @kentcdodds is okay with it. Let me know if you need help with anything.

ghostd · 2020-07-24T17:23:23Z

Here are the rest of the migration (only "src", not "tests") before my vacation.

I think we can remove dtslint now the source uses TypeScript (and add the .d.ts files into .gitignore).

kentcdodds · 2020-07-24T19:09:42Z

Thanks @ghostd.

Obviously there are a handful of merge conflicts which will need to be resolved before this is merged, but I'd like it if @testing-library/core-maintainers could review this, provide feedback, and we need to decide whether this is something we want to pursue.

marcosvega91 · 2020-07-25T10:49:51Z

Thanks @ghostd.

Obviously there are a handful of merge conflicts which will need to be resolved before this is merged, but I'd like it if @testing-library/core-maintainers could review this, provide feedback, and we need to decide whether this is something we want to pursue.

I'm a great fun of Typescript but sometimes it requires more time to understand and use, especially when types used are very restrictive. It could help to avoid some errors but at the same time we have a lot of tests to avoid them.
Typescript is also known by less people than JavaScript so it could be more difficult for newcomers to contribute to this library.

kentcdodds · 2020-07-25T14:51:23Z

it could be more difficult for newcomers to contribute to this library.

That's precisely why I've not added TypeScript to any of my open source projects.

alexkrolick · 2020-07-27T19:31:02Z

src/get-queries-for-element.ts

@@ -0,0 +1,55 @@
+import * as defaultQueries from './queries'
+


This area seems the most complex in the migraiton... I know we've gone through a few iterations on the typings for this. Hopefully it doesn't need to change often.

alexkrolick · 2020-07-27T19:32:14Z

src/matches.ts

-function fuzzyMatches(textToMatch, node, matcher, normalizer) {
+export type MatcherFunction = (content: string, element: HTMLElement) => boolean
+
+export type Matcher = string | RegExp | MatcherFunction


The types for these options are quite helpful

src/matches.ts

src/queries/display-value.ts

-  {exact = true, collapseWhitespace, trim, normalizer} = {},
+  container: HTMLElement,
+  value: Matcher,
+  {exact = true, collapseWhitespace, trim, normalizer}: MatcherOptions = {},


alexkrolick · 2020-07-27T19:45:54Z

src/queries/label-text.ts

@@ -35,13 +38,19 @@ function queryAllLabelsByText(
    })

    return matcher(textToMatch, label, text, matchNormalizer)
-  })
+  }) as HTMLElement[]


This isn't inferrable? I wonder what the type is without the as, when you filter a NodeList.

This is widening the type from HTMLLabelElement[]. Though I don't know why this is intended.

Actually a nice constraint would be to add a queryAll function type that has HTMLElement[] as the return type. Then if we mess up the implementation to return something else, it would break. Something like this:

function queryAllLabelsByText(/* */): QueryAllResult { }

I'm not sure about the widening part.

Returning an HTMLLabelElement if HTMLElement is expected is perfectly fine though since HTMLLabelElement extends HTMLElement. The other way around would be problematic.

MichaelDeBoey · 2020-11-26T15:39:41Z

kcd-scripts is now includes it's own typecheck script & auto-generates type files 🥳🎉

https://github.com/kentcdodds/kcd-scripts/releases/tag/v7.2.0

marcosvega91 · 2020-11-26T21:31:01Z

Maeby we can start migrating @testing-library/react that is smaller and than we can try with this ?

nickserv · 2020-11-28T11:06:30Z

I think it would still be best to start here, as most packages (including React Testing Library and User Event) depend on it. If we migrate dependent packages first, and then change the types emitted in DOM Testing Library, then we may have to update the dependent packages again.

Finishing migrating DOM Testing Library (including using this script and resolving merge conflicts) first may be more work up front, but I think it will be less work in the long run and it will make it easier to keep types correct when migrating dependent packages. Let me know if any help is needed.

kentcdodds · 2020-11-29T03:23:06Z

I think it would be best to avoid doing the migration all at once and instead do it over time and only start generating the type defs when we're finished.

nickserv · 2020-11-29T04:22:37Z

Does that mean that we're updating or closing this pull request? I may be able to help but I'm not sure where to continue.

kentcdodds · 2020-11-29T04:27:53Z

Probably much easier to close at this point

Proposal setup to make the TypeScript migration easier

b6d0e2c

Fix lint (will need a proper eslint settings for TS)

f11ac62

kentcdodds reviewed Jun 9, 2020

View reviewed changes

babel.config.js Outdated Show resolved Hide resolved

Migrate event-map.js and matches.js

53cb86d

Disable some tslint rules which conflicts with the declarations generated by tsc Also remove the useless babel.config.js

kentcdodds reviewed Jun 9, 2020

View reviewed changes

ghostd mentioned this pull request Jun 10, 2020

Allow JSX and TS file extensions into Jest config kentcdodds/kcd-scripts#153

Merged

3 tasks

Add the 'types' directory into .prettierignore

65ef080

It seems the prettier settings can conflit with the 'whitespace' rule of dtslint. Also forces the dtslint version to be sure Travis use a version with this fix: microsoft/dtslint#295

eps1lon reviewed Jun 10, 2020

View reviewed changes

types/event-map.d.ts Outdated Show resolved Hide resolved

Format the 'types' directory

c9340dd

kentcdodds previously approved these changes Jun 10, 2020

View reviewed changes

smeijer reviewed Jun 11, 2020

View reviewed changes

ghostd added 3 commits June 12, 2020 22:36

helper.ts

83cf94e

pretty-dom.ts

ecfb903

config.ts

01134b6

kentcdodds mentioned this pull request Jun 13, 2020

feat(userEvent): build-in @testing-library/user-event #616

Closed

4 tasks

kentcdodds mentioned this pull request Jul 2, 2020

Convert codebase to TypeScript #494

Open

ghostd added 5 commits July 16, 2020 21:22

events.ts / event-map.ts

d771b65

wait-for.ts

8a79b4c

wait-for-element.ts

f8becd7

wait-for-element-to-be-removed

62b98d8

role-helpers

28e4b18

ghostd added 3 commits July 24, 2020 17:06

suggestions

b4e8342

Migrate the rest of src (not stable yet)

385625e

Generate types during the build target

3e8c451

ghostd dismissed kentcdodds’s stale review via 3e8c451 July 24, 2020 17:20

alexkrolick reviewed Jul 27, 2020

View reviewed changes

src/matches.ts Show resolved Hide resolved

alexkrolick reviewed Jul 27, 2020

View reviewed changes

src/queries/display-value.ts

{exact = true, collapseWhitespace, trim, normalizer} = {},

container: HTMLElement,

value: Matcher,

{exact = true, collapseWhitespace, trim, normalizer}: MatcherOptions = {},

This comment was marked as off-topic.

Sign in to view

alexkrolick reviewed Jul 27, 2020

View reviewed changes

kentcdodds closed this Nov 29, 2020

This was referenced Dec 10, 2020

feat: migrate some files to typescript #848

Merged

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

Open

	const EventConstructor = window[EventType] \|\| window.Event
	let event
	/* istanbul ignore else */
	if (typeof EventConstructor === 'function') {
	event = new EventConstructor(eventName, eventInit)
	} else {
	// IE11 polyfill from https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent#Polyfill
	event = window.document.createEvent(EventType)

Proposal setup to make the TypeScript migration easier #614

Proposal setup to make the TypeScript migration easier #614

Conversation

ghostd commented Jun 9, 2020 • edited Loading

codesandbox-ci bot commented Jun 9, 2020 • edited Loading

ghostd commented Jun 9, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

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

codecov bot commented Jun 10, 2020 • edited Loading

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

smeijer Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smeijer Jun 11, 2020 • edited Loading

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

smeijer Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

smeijer Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jun 13, 2020

nickserv commented Jun 13, 2020 • edited Loading

kentcdodds commented Jun 14, 2020

nickserv commented Jun 14, 2020

nickserv commented Jun 16, 2020 • edited Loading

ghostd commented Jul 24, 2020

kentcdodds commented Jul 24, 2020

marcosvega91 commented Jul 25, 2020

kentcdodds commented Jul 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

alexkrolick Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkrolick Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelDeBoey commented Nov 26, 2020

marcosvega91 commented Nov 26, 2020

nickserv commented Nov 28, 2020 • edited Loading

kentcdodds commented Nov 29, 2020

nickserv commented Nov 29, 2020 • edited Loading

kentcdodds commented Nov 29, 2020

ghostd commented Jun 9, 2020 •

edited

Loading

codesandbox-ci bot commented Jun 9, 2020 •

edited

Loading

codecov bot commented Jun 10, 2020 •

edited

Loading

smeijer Jun 11, 2020 •

edited

Loading

smeijer Jun 11, 2020 •

edited

Loading

smeijer Jun 12, 2020 •

edited

Loading

smeijer Jun 11, 2020 •

edited

Loading

nickserv commented Jun 13, 2020 •

edited

Loading

nickserv commented Jun 16, 2020 •

edited

Loading

alexkrolick Jul 27, 2020 •

edited

Loading

alexkrolick Jul 27, 2020 •

edited

Loading

nickserv commented Nov 28, 2020 •

edited

Loading

nickserv commented Nov 29, 2020 •

edited

Loading