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

Framework: Resolve WordPress package type imports #18927

Merged
merged 2 commits into from Dec 13, 2019
Merged

Conversation

@aduth
Copy link
Member

aduth commented Dec 5, 2019

Previously suggested by @dsifford at #18838 (comment)

This pull request seeks to add necessary configuration to resolve cross-package type imports.

In Progress: This may be problematic to implement until existing issues can be resolved, since an import of a type will subject the entire dependency hierarchy to types checking, even if the module is not part of the includes set in the configuration.

Testing Instructions:

As an example, include a type reference to a file covered by types checking:

diff --git a/packages/url/src/index.js b/packages/url/src/index.js
index e39d95f28..d6af6b547 100644
--- a/packages/url/src/index.js
+++ b/packages/url/src/index.js
@@ -1,3 +1,5 @@
+/** @type {import('@wordpress/element').WPComponent} */
+
 export { isURL } from './is-url';
 export { isEmail } from './is-email';
 export { getProtocol } from './get-protocol';

If you have an editor integration, you can observe that the type is resolved:

image

However, when running npm run lint-types, you will also see ~26 new errors introduced through the analyzed result of the @wordpress/element dependency hierarchy.

tsconfig.json Show resolved Hide resolved
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 5, 2019

/** @type {import('@wordpress/element').WPComponent} */

Isn't this type defined in the src/react.js file rather than in src/index.js as configured?

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 5, 2019

/** @type {import('@wordpress/element').WPComponent} */

Isn't this type defined in the src/react.js file rather than in src/index.js as configured?

Hm, that's a good observation. I wonder if TypeScript is traversing all of the dependency hierarchy, which could be problematic for what I propose in #18927 as being a "public API" of types. I mean, at worst it means we expose all types of a package, which I suppose is not too problematic, and it can make it easier to avoid needing to "re-export" all of these types from the entry point. The only worry I'd have is being clear that if a type is associated with a function which is otherwise not part of the public API, there should be no commitment to backwards-compatibility, whatever that might mean to have "backwards-compatibility of a type".

@dsifford

This comment has been minimized.

Copy link
Contributor

dsifford commented Dec 5, 2019

@aduth Do you have the typescript definitions installed from definitelytyped? If so, it might also be coming from there.

Your editor might automatically be fetching those definitions from definitelytyped even if the repo doesn't have them installed as well.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 5, 2019

I was seeing this in my earlier iterations of this branch, how the modules would resolve to a cached copy of the DefinitelyTyped types. I don't think that's what's happening here. I'd want to double-check, though.

@aduth aduth mentioned this pull request Dec 12, 2019
tsconfig.json Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>
@aduth aduth merged commit 112816a into master Dec 13, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the add/resolve-ts-wp-deps branch Dec 13, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.