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

@wordpress/element typings reference @types/react-dom, but it doesn't have it as a dependency #23130

Closed
marekdedic opened this issue Jun 13, 2020 · 9 comments · Fixed by #25086
Assignees
Labels
Needs Technical Feedback Needs testing from a developer perspective. npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress

Comments

@marekdedic
Copy link
Contributor

Describe the bug
Hi. I am developing WordPress plugins using TypeScript and after the recent deprecation of @types/wordpress__element, I tried to switch to the type definitions included in @wordpress/element - I've installed @wordpress/element as a dev-dependency and included the following type declaration in my project:

declare const wp: {
	components: typeof import('wordpress__components'); // That's how it was done before, for reference
	element: typeof import('@wordpress/element');
};

However, I got the error

/home/user/project/node_modules/@wordpress/element/build-types/react-platform.d.ts(1,30): error TS7016: Could not find a declaration file for module 'react-dom'. '/home/user/project/node_modules/react-dom/index.js' implicitly has an 'any' type.
  Try `npm install @types/react-dom` if it exists or add a new declaration (.d.ts) file containing `declare module 'react-dom';`

Upon closer inspection, I've found out that the type definitions from @wordpress/element reference types for react-dom, but it has @types/react-dom only as a dev-dependency. I believe this is a bug and @types/react-dom should be a dependency for @wordpress/element so that the type definitions are complete.

  • @wordpress/element version: 2.14.0
  • OS: Debian testing

Thanks!

@gziolo
Copy link
Member

gziolo commented Jun 14, 2020

@sirreal, @aduth or @dsifford - what would be your recommendation? It seems like every package that uses external types should list them as dependencies. Technically they are dev dependencies but in practice they won’t be installed when defined as such.

@gziolo gziolo added Needs Technical Feedback Needs testing from a developer perspective. npm Packages Related to npm packages labels Jun 14, 2020
@sirreal
Copy link
Member

sirreal commented Jun 15, 2020

I'm unable to reproduce this issue.

@marekdedic can you share more information? TypeScript version, tsconfig, etc.

Here's a working codesandbox including this code:

import * as element from "@wordpress/element";

const wp: {
  element: typeof import("@wordpress/element");
} = { element };

wp.element.render(
  wp.element.createElement("div", {}, "Hello, world!"),
  document.getElementById("app")
);

@sirreal
Copy link
Member

sirreal commented Jun 15, 2020

Technically they are dev dependencies but in practice they won’t be installed when defined as such.

I'm not certain of the best approach or recommended practices here. My thinking has been that the published types expose the public API. Generally, the public API doesn't expose the types of our dependencies. React is a bit of a special case and I can imagine it leaking quite easily, but as I mention above, I haven't been able to observe this in my testing.

@marekdedic
Copy link
Contributor Author

Hi,
it is skaut/skaut-google-drive-gallery#544 - in that PR, I had to add @types/react-dom to make it not fail.
Typescript is on version 3.9.5.

I do think there's one big difference in your example - the wp variable is actually assigned, whereas in my example, it is just declared in a .d.ts file, because it is provided by WordPress as a global on runtime...

@sirreal
Copy link
Member

sirreal commented Jun 18, 2020

I do think there's one big difference in your example - the wp variable is actually assigned, whereas in my example, it is just declared in a .d.ts file, because it is provided by WordPress as a global on runtime...

I did that for demonstration purposes, but I still don't see an issue using the code you provided.

import * as element from "@wordpress/element";

declare const wp: {
  components: typeof import("@wordpress/components");
  element: typeof import("@wordpress/element");
};

element.render(
  element.createElement("div", {}, "Hello, world!"),
  document.getElementById("app")
);

@dsifford
Copy link
Contributor

dsifford commented Jun 18, 2020

Butting in real quick without doing much diligence to mention that the issue could also be due to a package-lock with stale nested dependencies.

Does the issue replicate if you install without a package-lock @marekdedic ?

@marekdedic
Copy link
Contributor Author

Does the issue replicate if you install without a package-lock @marekdedic ?

Yes it does unfortunately. I haven't had the time to make a minimal example, but in this PR, you can see the issue in action - skaut/skaut-google-drive-gallery#550

Hope this helps

@marekdedic
Copy link
Contributor Author

@sirreal I recreated your CodePen in a gist (because I could not figure out how to run a codepen...). The issue is still there

https://gist.github.com/marekdedic/01381b3047e35124d09863e8b408de8a

run:

$ npm install
$ npm run build

@sirreal
Copy link
Member

sirreal commented Sep 4, 2020

Thanks, I was able to reproduce with your example @marekdedic. I've opened #25086 to address.

sirreal added a commit that referenced this issue Sep 4, 2020
sirreal added a commit that referenced this issue Sep 7, 2020
* Add @types/react and @types/react-dom dependency to element
  Fixes #23130
* Upgrade the type packages
* Add changelog
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. npm Packages Related to npm packages [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants