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

Exploration: Refactor codebase to TypeScript #191

Closed
wants to merge 48 commits into from

Conversation

fabiankaegy
Copy link
Member

@fabiankaegy fabiankaegy commented Jan 17, 2023

Description of the Change

This PR aims to refactor the entire codebase of the @10up/block-components to use TypeScript. The reason for using TypeScript is to provide consumers of the Block Components package with much better IDE integration and autocomplete when using any of the components/hooks from this package. Additionally, it allows us to gain more confidence in the code of this library without relying on the current PropTypes workflow.

How to test the Change

All the tests should continue to pass. Additionally, you can go into the example/src/blocks/ folder and try out the better autocomplete/IDE integration this change would entail for the library users.

Changelog Entry

Changed - Refactor block components to use TypeScript internally

Credits

Props @fabiankaegy, @Antonio-Laguna, @ravinderk

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@fabiankaegy fabiankaegy self-assigned this Jan 17, 2023
@github-actions
Copy link

github-actions bot commented Jan 18, 2023

Size Change: +2.75 kB (+5%) 🔍

Total Size: 56.3 kB

Filename Size Change
dist/index.js 56.3 kB +2.75 kB (+5%) 🔍

compressed-size-action

@fabiankaegy
Copy link
Member Author

Hey @Antonio-Laguna @nicholasio @tlovett1

I would love to get your thoughts on this exploration. My goal is to ship better IDE integration and autocomplete for anyone using the Block Components. But I'm unsure whether using TypeScript would hinder more folks from contributing to the library.

I would consider myself to be a TypeScript noob (as you can probably quickly tell by the code in this PR 😂 ). But even with the naive implementation here it took me a few hours to convert the entire codebase over to TS, and I didn't need to change much. The few things I did change are actually potential bugs anyways so the changes improve the stability of the library itself.

I would love to get your all's thoughts on the topic.

Thanks in advance :)

(And just to be clear, my intention here is not to slowly convert the actual code we write for blocks to TS. I just think it might be beneficial for us in this Library to make it easier for anyone who consumes it even in regular JS contexts)

@fabiankaegy
Copy link
Member Author

I will say that we are still not at the point where all of the WordPress core components ship with proper types. Some of the packages already are written in TS or have external type definitions. But there still is a way to go which will mean out types will not always be fully complete out of the box if we just extend a core component.

Copy link
Member

@Antonio-Laguna Antonio-Laguna left a comment

Choose a reason for hiding this comment

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

Hey Fabian!

I love this as I said in private!

Here are some comments:

On registerIcons, I would update to something like:

type Icon = {
	source: string;
	name: string;
	label: string;
};

export function registerIcons(options: Icon[]) {
	dispatch(iconStore).registerIconSet(options);
}

That way, whenever you try to pass something that does not belong, you would get yelled:

TS2345: Argument of type '{ source: string; name: string; }' is not assignable to parameter of type 'Icon[]'.   Object literal may only specify known properties, and 'source' does not exist in type 'Icon[]'.

I'd also add types to more places such as PickedItem functions so that if you use the element, you get the documentation.

I'm happy to jump here and help you with this or can also offer more direct feedback on the areas. Whatever you prefer!

This is a great addition!

components/color-settings/index.tsx Outdated Show resolved Hide resolved
components/content-picker/PickedItem.tsx Show resolved Hide resolved
components/icon-picker/inline-icon-picker.tsx Outdated Show resolved Hide resolved
components/inner-block-slider/icons.tsx Outdated Show resolved Hide resolved
@fabiankaegy
Copy link
Member Author

@Antonio-Laguna Thanks so much for your feedback! I addressed your immediate comments :)

If you have the time to dive in and improve the types / add more that would be great :) But this isn't a top priority to no rush :)

*/
export function useOnClickOutside(onClickOutside) {
const ref = useRef();
export function useOnClickOutside(onClickOutside: (event: MouseEvent | TouchEvent) => void) {
Copy link
Member

Choose a reason for hiding this comment

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

@fabiankaegy By typing the event object with React.MouseEvent<HTMLElement> | React.TouchEvent<HTMLElement> types you can leverage React's synthetic event system.

fabiankaegy and others added 12 commits July 10, 2023 13:57
Co-authored-by: Gabriel Manussakis <[email protected]>
Co-authored-by: Gabriel Manussakis <[email protected]>
# Conflicts:
#	components/inner-block-slider/index.tsx
#	package-lock.json
#	package.json
# Conflicts:
#	components/inner-block-slider/index.tsx
# Conflicts:
#	components/content-search/SearchItem.tsx
#	components/content-search/index.tsx
#	components/icon-picker/icon-picker.tsx
#	components/icon-picker/icon.tsx
#	package-lock.json
#	package.json
@cypress
Copy link

cypress bot commented Aug 24, 2023

Passing run #594 ↗︎

0 6 0 0 Flakiness 0

Details:

fix: make labels property optional and set default value to properties
Project: 10up Block Components Commit: 7ff5f4d178
Status: Passed Duration: 02:57 💡
Started: Aug 25, 2023 11:54 AM Ended: Aug 25, 2023 11:57 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@fabiankaegy
Copy link
Member Author

I merged the base typescript config to the main branch. So we can now create smaller incremental updates to the various components. Therefore I will close this large PR for now.

Thank you to anyone that helped contribute to this PR. It has been incredibly helpful to figure out what to do and now we can actually apply our learnings by creating small individual PR's for the carious components

@ravinderk ravinderk deleted the exploration/typescript branch October 23, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants