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

Element: Add types #21248

Merged
merged 14 commits into from
Apr 4, 2020
Merged

Element: Add types #21248

merged 14 commits into from
Apr 4, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Mar 29, 2020

Description

Add types/typechecking to @wordpress/element. Many other packages that would be extremely valuable to type, namely @wordpress/data and @wordpress/components, depend on this package. Typing it unblocks a lot of work on other package typings.

  • Improve internal JSDoc/TypeScript typings.
  • Add published types.

Add unknown and never TypeScript types to allowed JSDoc types.

Includes squash-merged #20669, necessary for some required types.

Part of #18838

How has this been tested?

npm run build:package-types passes.

Screenshots

Types of changes

New feature: Add types

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@sirreal sirreal self-assigned this Mar 29, 2020
@github-actions
Copy link

github-actions bot commented Mar 29, 2020

Size Change: -3 B (0%)

Total Size: 889 kB

Filename Size Change
build/element/index.js 4.44 kB -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.4 kB 0 B
build/api-fetch/index.js 3.79 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 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/blocks/index.js 57.5 kB 0 B
build/components/index.js 195 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.04 kB 0 B
build/data/index.js 8.23 kB 0 B
build/date/index.js 5.36 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/index.js 2.71 kB 0 B
build/edit-navigation/style-rtl.css 239 B 0 B
build/edit-navigation/style.css 241 B 0 B
build/edit-post/index.js 92.9 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.1 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.18 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/editor/index.js 42.8 kB 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/index.js 6.95 kB 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/i18n/index.js 3.57 kB 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/notices/index.js 1.57 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/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 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.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Comment on lines +7 to +8
"noImplicitAny": false,
"strictNullChecks": false
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing these lines:

  • noImplicitAny -> 51 errors
  • strictNullChecks -> 7 errors
  • both -> 60 errors

@sirreal sirreal force-pushed the try/type-wp-element branch from ca897c8 to c3549e2 Compare March 29, 2020 22:25
@sirreal sirreal marked this pull request as ready for review March 29, 2020 22:34
@sirreal sirreal added [Package] Element /packages/element [Type] Code Quality Issues or PRs that relate to code quality labels Mar 29, 2020
@sirreal sirreal requested a review from dsifford March 30, 2020 09:28
@sirreal sirreal force-pushed the try/type-wp-element branch from 7088eae to d3fe07e Compare March 31, 2020 19:00
@sirreal
Copy link
Member Author

sirreal commented Mar 31, 2020

Rebased and removed commits from #20669 which has been merged.

packages/element/README.md Outdated Show resolved Hide resolved
packages/element/src/create-interpolate-element.js Outdated Show resolved Hide resolved
packages/element/src/serialize.js Outdated Show resolved Hide resolved
packages/eslint-plugin/configs/jsdoc.js Show resolved Hide resolved
packages/element/src/serialize.js Show resolved Hide resolved
packages/element/src/serialize.js Show resolved Hide resolved
@sirreal sirreal force-pushed the try/type-wp-element branch from 06b1bf9 to 8bed5cf Compare April 3, 2020 20:45
@sirreal
Copy link
Member Author

sirreal commented Apr 3, 2020

Rebased to fix conflict. Would it be possible to move forward with this? Many packages depend on element so it's a a bottleneck.

@sirreal sirreal added the Needs Technical Feedback Needs testing from a developer perspective. label Apr 3, 2020
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

@sirreal sirreal merged commit fe0d10c into master Apr 4, 2020
@sirreal sirreal deleted the try/type-wp-element branch April 4, 2020 20:57
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 4, 2020
This was referenced Apr 8, 2020
Comment on lines -16 to +20
* @return {WPComponent} Dangerously-rendering component.
* @return {JSX.Element} Dangerously-rendering component.
Copy link
Member

Choose a reason for hiding this comment

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

I wondered how this worked.

Turns out it doesn't:

[0] /home/travis/build/WordPress/gutenberg/packages/element/src/raw-html.js
[0]   20:0  warning  The type 'JSX' is undefined  jsdoc/no-undefined-types

https://travis-ci.com/github/WordPress/gutenberg/jobs/314225929

I thought these rules were upgraded to be errors in #20427. I think this one might have been missed? (cc @sainthkh)

I know we have a handful of temporary exceptions for custom types, but pretty sure those are hard-coded to be exceptions and could still exist even if the rule were upgraded from warning to error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a concern with restoring the element types? #21781

Copy link
Contributor

Choose a reason for hiding this comment

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

It happened because I missed this one rule:

'jsdoc/no-undefined-types': [
'warn',

I opened an issue that fixes it: #21942.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Package] Element /packages/element [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants