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

Publish block editor build types #49969

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

noahtallen
Copy link
Member

What?

Publishes block editor build types. (This package is a lot bigger, and I think the types published are going to be pretty helpful.)

Why?

See #49647

How?

Testing Instructions

  • npm run build:package-types
  • check types at packages/block-editor/build-types

@noahtallen noahtallen added [Type] Build Tooling Issues or PRs related to build tooling [Package] Block editor /packages/block-editor labels Apr 21, 2023
@noahtallen noahtallen self-assigned this Apr 21, 2023
@noahtallen
Copy link
Member Author

Currently down to just a couple errors:

Incorrect published types for /Users/noahallen/source/gutenberg/packages/block-editor/build-types/index.d.ts:
 packages/block-editor/build-types/components/block-list/use-block-props/index.d.ts(21,14): error TS2552: Cannot find name 'getBlockProps'. Did you mean 'useBlockProps'?
packages/block-editor/build-types/components/inner-blocks/index.d.ts(29,14): error TS2552: Cannot find name 'getInnerBlocksProps'. Did you mean 'useInnerBlocksProps'?

These are a little tricky. Any suggestions? This the source code for one of those errors, but they are both the same:

import {
getBlockSupport,
store as blocksStore,
__unstableGetInnerBlocksProps as getInnerBlocksProps,
} from '@wordpress/blocks';

useInnerBlocksProps.save = getInnerBlocksProps;

@@ -12,6 +12,10 @@ import { chevronRightSmall, Icon } from '@wordpress/icons';
import BlockTitle from '../block-title';
import { store as blockEditorStore } from '../../store';

/**
* @typedef {import('@wordpress/element').WPElement} WPElement
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically all the issues in block-editor were just that we weren't importing types like this.

@@ -13,8 +13,6 @@ import { store as blockEditorStore } from '../../store';
import { BlockRefsProvider } from './block-refs-provider';
import { unlock } from '../../lock-unlock';

/** @typedef {import('@wordpress/data').WPDataRegistry} WPDataRegistry */
Copy link
Member Author

@noahtallen noahtallen Apr 21, 2023

Choose a reason for hiding this comment

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

this appears to be unused but was causing an error

@@ -30,7 +30,8 @@ import type { ViewProps } from './types';
// @ts-expect-error
export const View: WordPressComponent<
'div',
ViewProps & RefAttributes< any >
ViewProps & RefAttributes< any >,
true
Copy link
Member Author

Choose a reason for hiding this comment

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

temporary fix so that I could see other type errors. (This will be solved by #49960)

@adamziel
Copy link
Contributor

Cc @johnhooks

@johnhooks
Copy link
Contributor

johnhooks commented Apr 21, 2023

@noahtallen I have two PRs that could be helpful in typing the block editor.

  • feature: add TypeScript types to the blocks package #48604 - It's a pretty huge PR and I need some help wrapping it up and finding support to get it merge it. But it would be a great improvement and help improve the types of the block editor. There are going to be lots of issues to resolve before we could enable checkJs because the code wasn't written with types in mind, but the exported types great for packages that depend on @wordpress/blocks.
  • feature(data): add types to dispatch and select #49930 - This could help if you are using dispatch and select to access the block editor store.

Also, I'm very willing to help type the block editor, just point me in the right direction.

@noahtallen
Copy link
Member Author

Very nice! My goal with this is to basically let us publish types without requiring checkJs. I added a script in #49736 which helps us avoid publishing any errors.

After discussion with @dmsnell, @tyxla, and @adamziel, my thought process is that we still get a ton of value by publishing what we can, even if it’s inferred from JSDoc. It’s hard to maintain DT, and if we publish here, we can remove that from the equation. Then, we can continue improving types (which is the hardest part!) until checkJs could be enabled! So the published types gradually get better and better — but we still get a lot out of the box (like full types for the store, which is harder to do in DT!)

I think the only thing blocking this PR now is the error I mentioned above.

but I’ll also take a look at those PRs!

@noahtallen
Copy link
Member Author

Deviations from DT are probably the biggest unknown. In some packages, I don’t think this is a big enough issue to block things. Maybe that’s different since block editor is so big 🤔

@tyxla
Copy link
Member

tyxla commented Apr 24, 2023

Currently down to just a couple errors:

Incorrect published types for /Users/noahallen/source/gutenberg/packages/block-editor/build-types/index.d.ts:
 packages/block-editor/build-types/components/block-list/use-block-props/index.d.ts(21,14): error TS2552: Cannot find name 'getBlockProps'. Did you mean 'useBlockProps'?
packages/block-editor/build-types/components/inner-blocks/index.d.ts(29,14): error TS2552: Cannot find name 'getInnerBlocksProps'. Did you mean 'useInnerBlocksProps'?

These are a little tricky. Any suggestions? This the source code for one of those errors, but they are both the same:

import {
getBlockSupport,
store as blocksStore,
__unstableGetInnerBlocksProps as getInnerBlocksProps,
} from '@wordpress/blocks';

useInnerBlocksProps.save = getInnerBlocksProps;

I haven't spent a bunch of time on this one, but it might mean that we might need to publish types for the @wordpress/blocks package first.

@johnhooks
Copy link
Contributor

johnhooks commented Apr 24, 2023

I'll try merging this into a fork of #48604 and see if it helps.

Edit:

@tyxla you were right the issue was @wordpress/blocks not being typed. Merging this PR with #48604 worked. I had to make a few adjustments because we removed the WP prefix from the types exported from the @wordpress/blocks package.

@tyxla
Copy link
Member

tyxla commented Apr 25, 2023

Thanks for verifying @johnhooks! Seems like we should land those PRs in a sequence.

Besides the memize update, is there anything else that is blocking #48604?

@johnhooks
Copy link
Contributor

@tyxla I'm currently reviewing the entire PR, I hope to have it cleaned up by the end of the week.

I was also attempting to type the internals of the package, though this will be an on going process. Should I leave those changes or remove them and save it for later, concentrating on just providing exported types?

@tyxla
Copy link
Member

tyxla commented Apr 25, 2023

Sounds great, thanks @johnhooks!

I was also attempting to type the internals of the package, though this will be an on going process. Should I leave those changes or remove them and save it for later, concentrating on just providing exported types?

My suggestion would be to leave the internals for another time and focus on the exported types. That can also be a good way to split the work into more bearable and easy-to-grok and easy-to-land pieces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants