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

eui.d.ts has undeclared dependency imports #1959

Closed
thompsongl opened this issue May 23, 2019 · 5 comments · Fixed by #2828
Closed

eui.d.ts has undeclared dependency imports #1959

thompsongl opened this issue May 23, 2019 · 5 comments · Fixed by #2828

Comments

@thompsongl
Copy link
Contributor

Type checking in elastic/gatsby-eui-starter#7 finds that @types/react-virtualized and @types/enzyme are imported in eui.d.ts but not declared in its dependencies or peerDependencies (gatsby-eui-starter uses neither).

@types/react-virtualized seems like it should simply move to dependencies as it's used directly in src.
I'm less certain about what to do with the exposed enzyme test helpers.

Kibana covers these holes because it uses both libraries.
Cloud uses enzyme and specifiesstrict: false in its tsconfig and so gets past the react-virutalized warning.

I'll get thoughts from @chandlerprall before moving forward with anything.

@chandlerprall
Copy link
Contributor

react-beautiful-dnd as well

@snide
Copy link
Contributor

snide commented Sep 12, 2019

Is this still an issue? I noticed react-virtualized was moved. I haven't had any issues in TS projects with Gatsby recently.

@chandlerprall
Copy link
Contributor

It is still an issue (at least the concept, if not the specific packages) - I hit this whenever I spin up a quick test environment for EUI builds.

@thompsongl
Copy link
Contributor Author

thompsongl commented Jan 29, 2020

@snide was working through upgrading deps in the elastic/gatsby-eui-starter, which brought this back to mind.

Easy changes: @types/react-beautiful-dnd has already been moved , so @types/react-virtualized and @types/enzyme need to move to be dependencies of EUI. They're used in exported modules (@types/enzyme only; enzyme proper is not necessary).

Discussion: @elastic/charts is now of the same middling dependency type as the above. Types from @elastic/charts (PartialTheme, LineAnnotationStyle) are used in exported theme modules.
Do we add @elastic/charts as a peerDep or dep? Do we find a workaround to avoid the dependency?

@chandlerprall
Copy link
Contributor

Would be great to avoid listing @elastic/charts as a dep. Since themes.tsx isn't exported by the top-level @elastic/eui, ideally we can find a simple way to ignore it being included in eui.d.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants