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

Types: Resolve conflicts with DefinitelyTyped #21767

Closed
sirreal opened this issue Apr 21, 2020 · 7 comments
Closed

Types: Resolve conflicts with DefinitelyTyped #21767

sirreal opened this issue Apr 21, 2020 · 7 comments
Assignees
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@sirreal
Copy link
Member

sirreal commented Apr 21, 2020

Summary

Some @wordpress/* emit TypeScript type declarations and include them with the published packages. Shortly after publishing, it was discovered the @wordpress/element conflicted with the DefinitelyTyped (DT) 3rd party type declarations of several packages and a patch release was published removing the published types from @wordpress/element, @wordpress/primitives, and @wordpress/icons (#21613).

This issue is for the purpose of laying out the problem and discussing paths forward.

The problem

One major purpose of @wordpress/element is an abstraction over React. In many ways, it is a very thin layer that re-exports React functionality.

Third party types on DT expose all of the React types. The implementation in the @wordpress/element package is selective about what it exposes from React.

This means that the DT types for @wordpress/element exposes all of the React types, and other @wordpress/* DT types make use of react via @wordpress/element. For example, the ComponentType in @wordpress/blocks DT package comes from @wordpress/element's DT types, but is just React's ComponentType.

When the 1st party @wordpress/element types were published, they superseded the DT types. They do not include the re-exports of React types. This causes a cascade of issues for other DT types depending on @wordpress/element for React types.

Considerations

In an ideal future, all packages provide 1st party type declarations which are verified by the TypeScript type system. The need for 3rd party types will go away and the types can be deprecated. There is a significant amount of work to do before we can realize this future.

In the interim, DT types provide value for packages that don't yet publish their own declarations.

Packages other than the DT types may rely on the @wordpress/element-provided React types.

Possible solutions

Please edit to add additional solutions. It's worth considering all paths forward.

Use React types directly from react

Rather than expose React types from @wordpress/element, @wordpress/element and other packages would reference React types directly.

Pros: Simplicity, clear path forward, less to maintain.
Cons: Exposes React directly to consumers, breaking the facade.

Ignore conflicts with DT

We could republish the @wordpress/element package with types as it is now. This is known to break some DT types and will affect consumers relying on those types.

Pros: Requires no effort.
Cons: Anyone relying on DT types for certain packages in negatively impacted.

We don't strictly have any obligation to maintain compatibility with DT types. I would discourage this course of action.

Expose React types from @wordpress/element

Just like @wordpress/element eposes a lot of functionality from React, types could also be exposed. In practice, this has proved to be difficult.

Pros: Maintain the facade in front of React.
Cons: A lot of work.

While TypeScript has clear ways of importing and exporting types, I was unable to re-export React types from @wordpress/element via JSDoc. At the very least, this would mean taking on a significant maintenance burden.

cc: @aduth @dsifford
Related: #18942 #18838

@sirreal sirreal added [Type] Code Quality Issues or PRs that relate to code quality [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. labels Apr 21, 2020
@sirreal
Copy link
Member Author

sirreal commented Apr 21, 2020

I've started work in DefinitelyTyped to access React types directly:

DefinitelyTyped/DefinitelyTyped#44086

@dsifford
Copy link
Contributor

dsifford commented Apr 21, 2020

Good writeup.

Right away I'm also wondering if the following two options could be put in the pool of considerations....

1. Don't hide React stuff behind the WP facade

This seems like it would be relatively simple to do, because this just affects the jsdoc. Everywhere you use WPComponent, just use React.Component, etc etc...

This wouldn't break anybody to my knowledge either.

2. (extreme option) Deprecate @wordpress/element

I'm not sure I fully understand the benefit of the @wordpress/element package in the first place, over just using the wordpress-bundled React? I understand that it hides away functionality that in most cases people shouldn't have access to (portals I think are one of these cases), but is it worth maintaining essentially a reduced-functionality fork of react for this?

Obviously, this is much easier said than done as now there are likely thousands of people dependent on @wordpress/element. There'd definitely need to be some sort of deprecation/migration path, but just another (extreme) option to consider.


Edit: I think your first option @sirreal might be the same thing as what my first option is suggesting. I read yours from the context of DT, and not the JSDoc. If that's the case, disregard.

Edit 2: I'm now remembering that @wordpress/element has a few extra goodies that it exports along with react, so deprecation likely wouldn't be possible without moving that functionality to another package (and the headache involved with that is prob not worth it)

@sirreal
Copy link
Member Author

sirreal commented Apr 21, 2020

1. Don't hide React stuff behind the WP facade

I think this does align with the first option in the description Use React types directly from react. Everyone relies on react types and they naturally align.

2. (extreme option) Deprecate @wordpress/element

It's worth discussing, but it's likely larger than we can tackle here 🙂

Some difficulty with types probably isn't sufficient to drop the facade, but it is another factor to consider.

@sirreal
Copy link
Member Author

sirreal commented Apr 22, 2020

DefinitelyTyped/DefinitelyTyped#44086 is ready, it's the first step for Use React types directly from react.

#21781 restores the types in affected packages (element, icons, primitives) and was used to generate the @wordpress/element types in DefinitelyTyped/DefinitelyTyped#44086. Merging it is the second step.

@dsifford
Copy link
Contributor

Approved the first-step PR. Should merge shortly(ish).

@sirreal sirreal self-assigned this Apr 22, 2020
@sirreal
Copy link
Member Author

sirreal commented Apr 27, 2020

DefinitelyTyped/DefinitelyTyped#44086 has been merged, which unblocks publishing @wordpress/element types again: #21781

sirreal added a commit that referenced this issue Apr 28, 2020
* Restore element, icons, primitives types
* Partially reverts #21613 (c2684b2)
* Reverts #21784 (74fcd4c)

Some `@wordpress/*` emit TypeScript type declarations and include them with the published packages. Shortly after publishing, it was discovered the `@wordpress/element` conflicted with the DefinitelyTyped 3rd party type declarations of several packages and a patch release was published removing the published types from `@wordpress/element`, `@wordpress/primitives`, and `@wordpress/icons` (#21613, #21784).

The DefinitelyTyped type declarations have been updated to remove conflicts with the included `@wordpress/element` types. The types can be published again.

See #21767 for more details.
@sirreal
Copy link
Member Author

sirreal commented Apr 28, 2020

#21781 has been merged. That should indicate the conclusion of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests

2 participants