Block Editor: Add type-checking for block editor (DOM utils only) #21362

aduth · 2020-04-02T19:03:15Z

Related: #21361
Partially addresses #18838

This pull request proposes to add a minimal TypeScript configuration for the @wordpress/block-editor module, with the intention of progressively integrating type-checking into this very large module. As proposed, it currently includes type-checking for a single file, packages/block-editor/src/utils/dom.js. This was used to help verify the fix in #21361.

Until more packages opt in to typing, it may not be possible to add type-checking for certain files in this package. The file included here works largely because it has no dependencies. For the purposes of #21361, I tried also to include src/components/block-list/block-wrapper.js. However, since this file has many dependencies on other WordPress packages (which aren't necessarily typed), there were many errors. I assumed this may have to do with the fact we need to add type-checking for "leaf" packages before consumers, though I may be mistaken.

Testing Instructions:

Verify types build without error:

npm run build:package-types

cc @sirreal Do you think this is a reasonable way to go about starting to add type details for large packages?

github-actions · 2020-04-02T19:09:53Z

Size Change: +1.31 kB (0%)

Total Size: 885 kB

Filename	Size	Change
`build/block-directory/index.js`	6.03 kB	-1 B
`build/block-editor/index.js`	102 kB	+143 B (0%)
`build/block-editor/style-rtl.css`	10.6 kB	-115 B (1%)
`build/block-editor/style.css`	10.6 kB	-118 B (1%)
`build/blocks/index.js`	57.5 kB	-1 B
`build/components/index.js`	195 kB	-4 B (0%)
`build/edit-navigation/index.js`	2.71 kB	+228 B (8%)	🔍
`build/edit-post/index.js`	92.5 kB	+229 B (0%)
`build/edit-post/style-rtl.css`	12.1 kB	+63 B (0%)
`build/edit-post/style.css`	12.1 kB	+64 B (0%)
`build/edit-site/index.js`	9.78 kB	+667 B (6%)	🔍
`build/edit-site/style-rtl.css`	4.68 kB	+70 B (1%)
`build/edit-site/style.css`	4.68 kB	+71 B (1%)
`build/editor/index.js`	42.8 kB	-3 B (0%)
`build/element/index.js`	4.44 kB	+1 B
`build/format-library/index.js`	6.95 kB	+1 B
`build/i18n/index.js`	3.57 kB	+1 B
`build/notices/index.js`	1.57 kB	+1 B
`build/priority-queue/index.js`	789 B	+9 B (1%)
`build/redux-routine/index.js`	2.84 kB	+1 B
`build/warning/index.js`	1.14 kB	-1 B

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.45 kB	0 B
`build/api-fetch/index.js`	3.8 kB	0 B
`build/autop/index.js`	2.59 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-library/editor-rtl.css`	7.22 kB	0 B
`build/block-library/editor.css`	7.22 kB	0 B
`build/block-library/index.js`	110 kB	0 B
`build/block-library/style-rtl.css`	7.53 kB	0 B
`build/block-library/style.css`	7.54 kB	0 B
`build/block-library/theme-rtl.css`	669 B	0 B
`build/block-library/theme.css`	671 B	0 B
`build/block-serialization-default-parser/index.js`	1.65 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/components/style-rtl.css`	16.6 kB	0 B
`build/components/style.css`	16.5 kB	0 B
`build/compose/index.js`	6.21 kB	0 B
`build/core-data/index.js`	10.7 kB	0 B
`build/data-controls/index.js`	1.03 kB	0 B
`build/data/index.js`	8.23 kB	0 B
`build/date/index.js`	5.37 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.05 kB	0 B
`build/edit-navigation/style-rtl.css`	239 B	0 B
`build/edit-navigation/style.css`	241 B	0 B
`build/edit-widgets/index.js`	4.43 kB	0 B
`build/edit-widgets/style-rtl.css`	3.74 kB	0 B
`build/edit-widgets/style.css`	3.74 kB	0 B
`build/editor/editor-styles-rtl.css`	423 B	0 B
`build/editor/editor-styles.css`	426 B	0 B
`build/editor/style-rtl.css`	3.49 kB	0 B
`build/editor/style.css`	3.49 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	1.93 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.3 kB	0 B
`build/keycodes/index.js`	1.7 kB	0 B
`build/list-reusable-blocks/index.js`	2.99 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	4.84 kB	0 B
`build/nux/index.js`	3.01 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.54 kB	0 B
`build/primitives/index.js`	1.5 kB	0 B
`build/rich-text/index.js`	14.5 kB	0 B
`build/server-side-render/index.js`	2.54 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.01 kB	0 B
`build/viewport/index.js`	1.6 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

packages/block-editor/src/utils/dom.js

sirreal

I left one suggestion. The type changes seem fine assuming the type checker is happy 🙂

Did you mean to add this to the root tsconfig.json as well so it's typechecked as part of the main types build? I'd recommend that as well.

packages/block-editor/package.json

aduth · 2020-04-02T21:51:31Z

Did you mean to add this to the root tsconfig.json as well so it's typechecked as part of the main types build? I'd recommend that as well.

I didn't know this was a requirement 😬 And the fact that I didn't know has me even more concerned!

Co-Authored-By: Jon Surrell <[email protected]>

sirreal · 2020-04-02T22:00:16Z

Did you mean to add this to the root tsconfig.json as well so it's typechecked as part of the main types build? I'd recommend that as well.

I didn't know this was a requirement 😬 And the fact that I didn't know has me even more concerned!

It's mentioned here, but this does seem to indicate that it should be more prominent 🙂

To opt-in to TypeScript tooling, packages should include a tsconfig.json file in the package root and add an entry to the root tsconfig.json references. The changes will indicate that the package has opted-in and will be included in the TypeScript build process.

The bullet list I added to #18838 may be better:

Add a tsconfig.json to the package root. The @wordpress/i18n package has a good example of a basic config.

Add "types": "build-types" to the package's package.json so that consumers will pick up the published types from the package.

Add the package to the root tsconfig.json references.

packages/block-editor/tsconfig.json

sirreal · 2020-04-03T09:00:20Z

This approach of opting-in file by file works well with the caveat that you covered well in the description:

Until more packages opt in to typing, it may not be possible to add type-checking for certain files … [This file] works largely because it has no dependencies. For the purposes of #21361, I tried also to include src/components/block-list/block-wrapper.js. … [This has] to do with the fact we need to add type-checking for "leaf" packages before consumers …

It's best to work from leaf packages to consumer packages in the package-by-package approach. In the file-by-file approach it's best to work from leaves in. In both cases it's because we'll need the dependencies typed before we can type the dependents.

aduth · 2020-04-03T14:58:53Z

It's mentioned here, but this does seem to indicate that it should be more prominent 🙂

The bullet list I added to #18838 may be better:

Yeah, in my case, I followed my previous work in #21053, which... is also wrong 😞

Following the documented process will likely yield better results!

Noted as well at #21053 (comment) , it would be nice if we could have some combination of enforcement / automation around all of the various tasks associated with a new package. Depending on how that would be implemented (e.g. in response to the addition of a tsconfig.json), it could be helpful.

This reverts commit d4640a5.

aduth · 2020-04-03T15:30:52Z

Did you mean to add this to the root tsconfig.json as well so it's typechecked as part of the main types build? I'd recommend that as well.

Added in 16e9a98.

aduth · 2020-04-03T15:35:43Z

It's mentioned here, but this does seem to indicate that it should be more prominent 🙂
The bullet list I added to #18838 may be better:

Yeah, in my case, I followed my previous work in #21053, which... is also wrong 😞

See #21381

aduth added 2 commits April 2, 2020 14:49

Block Editor: Add type-checking for block editor (DOM utils only)

23ab50c

Block Editor: Correct DOM parameter types from HTMLElement to Element

9416fa4

aduth added the [Package] Block editor /packages/block-editor label Apr 2, 2020

aduth requested review from ellatrix and youknowriad as code owners April 2, 2020 19:03

aduth requested a review from sirreal April 2, 2020 19:03

aduth mentioned this pull request Apr 2, 2020

IE11: browser crashes on pressing Enter to switch to edit mode #21276

Closed

aduth commented Apr 2, 2020

View reviewed changes

packages/block-editor/src/utils/dom.js Show resolved Hide resolved

youknowriad approved these changes Apr 2, 2020

View reviewed changes

aduth mentioned this pull request Apr 2, 2020

Testing: Add TypeScript types validation for modules #18838

Closed

75 tasks

sirreal approved these changes Apr 2, 2020

View reviewed changes

packages/block-editor/package.json Outdated Show resolved Hide resolved

Block Editor: Avoid referencing block-editor types from package.json

2ded9f4

Co-Authored-By: Jon Surrell <[email protected]>

sirreal reviewed Apr 3, 2020

View reviewed changes

packages/block-editor/tsconfig.json Outdated Show resolved Hide resolved

packages/block-editor/tsconfig.json Show resolved Hide resolved

aduth added 4 commits April 3, 2020 11:25

Block Editor: Add all dependencies in references

d4640a5

Revert "Block Editor: Add all dependencies in references"

7cac610

This reverts commit d4640a5.

Framework: Include block-editor for root package type-checking

16e9a98

Block Editor: List type-checked files by files property

7f4a5a8

aduth mentioned this pull request Apr 3, 2020

Testing: Include Prettier Config package in root type checking #21381

Merged

aduth merged commit b3038e1 into master Apr 3, 2020

aduth deleted the add/block-editor-type-dom-utils branch April 3, 2020 16:05

github-actions bot added this to the Gutenberg 7.9 milestone Apr 3, 2020

aduth mentioned this pull request May 7, 2020

Refactor reusable block edit component using hooks (and fix interactions with multiple instances of the same reusable block) #21427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block Editor: Add type-checking for block editor (DOM utils only) #21362

Block Editor: Add type-checking for block editor (DOM utils only) #21362

aduth commented Apr 2, 2020 •

edited

Loading

github-actions bot commented Apr 2, 2020 •

edited

Loading

sirreal left a comment

aduth commented Apr 2, 2020

sirreal commented Apr 2, 2020

sirreal commented Apr 3, 2020

aduth commented Apr 3, 2020

aduth commented Apr 3, 2020

aduth commented Apr 3, 2020

Block Editor: Add type-checking for block editor (DOM utils only) #21362

Block Editor: Add type-checking for block editor (DOM utils only) #21362

Conversation

aduth commented Apr 2, 2020 • edited Loading

github-actions bot commented Apr 2, 2020 • edited Loading

sirreal left a comment

Choose a reason for hiding this comment

aduth commented Apr 2, 2020

sirreal commented Apr 2, 2020

sirreal commented Apr 3, 2020

aduth commented Apr 3, 2020

aduth commented Apr 3, 2020

aduth commented Apr 3, 2020

aduth commented Apr 2, 2020 •

edited

Loading

github-actions bot commented Apr 2, 2020 •

edited

Loading