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

Remove unexposed functions from declaration files #3709

Closed
markov00 opened this issue Jul 8, 2020 · 2 comments · Fixed by #3715
Closed

Remove unexposed functions from declaration files #3709

markov00 opened this issue Jul 8, 2020 · 2 comments · Fixed by #3715
Assignees

Comments

@markov00
Copy link
Member

markov00 commented Jul 8, 2020

The generated declaration file eui.d.ts is exposing some utility functions used only for tests like takeMountedSnapshot

declare module '@elastic/eui/src/test/take_mounted_snapshot' {
	import { ReactWrapper } from 'enzyme';
	import { Component } from 'react';
	export const takeMountedSnapshot: (mountedComponent: ReactWrapper<{}, {}, Component<{}, {}, any>>) => ChildNode | null;
}

This requires to declare @types/enzyme in the deps instead of devDeps as you also noticed here: #2828

Do we really need these test utility to be exported as part of the public API?
I’m asking because this dependency can and is creating some resolution conflicts within @elastic/charts because of two different versions of @types/react. I've solved that updating reacts types here and there, but I think it can be also cleanly solved if EUI exposes only its own types.

In elastic-charts we are using to mark the dependencies we don't want in the generated files as @internals and use stripInternals in the tsconfig.json (it's something that at the moment is not documented, I don't have an idea on why it's not on the TS documentation anymore).

Some cleanup can also be possible with the other @types you have moved under deps

@chandlerprall @thompsongl

@thompsongl
Copy link
Contributor

thompsongl commented Jul 8, 2020

Some background:

Quite a few places in Kibana import from '@elastic/eui/src/test/' (findTestSubject, for the most part), so keeping the types available was the main consideration when deciding to move enzyme to be a dependency (as opposed to removing the exported types).
In reality, I think we see the @types/enzyme dependency for testing utility types as more of a peerDependency. Whereby you can choose to use the testing utilities if your project also uses enzyme/@types/enzyme.

@chandlerprall proposed generating a separate test d.ts file for opt-in test utility use, removing the types from EUI's main d.ts

@thompsongl thompsongl self-assigned this Jul 8, 2020
@thompsongl
Copy link
Contributor

Update: Kibana imports actually target '@elastic/eui/lib/test/' which means that the definitions don't match.
We may still want to create a new d.ts file, but it would be an enhancement and not to maintain compatibility.

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.

2 participants