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 src/test/* from type definitions file #3715

Merged
merged 7 commits into from
Jul 9, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jul 8, 2020

Summary

Closes #3709 by removing src/test/* files from eui.d.ts. Additionally generates new lib/test/index.d.ts and es/test/index.d.ts files specifically for consumers who use test directory utilities.

As src/test is not formally part of EUI's API, test imports occur from lib or es and are @ts-ignoreed. This PR is an enhancement for those imports, as types will automatically be picked up when importing from @elastic/eui/[lib, es]/test.

Removing src/test from eui.d.ts allows for @types/enzyme to be moved back to devDep status. Use of lib/test or es/test assumes your project maintains its own @types/enzyme dependency

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation
- [ ] Checked Code Sandbox works for the any docs examples
- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately

- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3715/

@chandlerprall
Copy link
Contributor

chandlerprall commented Jul 9, 2020

  • We should probably do the same for es/test
  • Instead of placing the new .d.ts in the top-level and requiring the project to add a configuration, it looks like we can put index.d.ts files in the lib/test&es/test dirs and typescript will automatically pick them up when importing that directory. This wouldn't allow imports from deeper directories, but we don't support that as a pattern from the lib/es directories either - i.e. everything has to start at the @elastic/eui entry.

@thompsongl
Copy link
Contributor Author

put index.d.ts files in the lib/test&es/test dirs and typescript will automatically pick them up

Yep, this is a better approach. Tested in Kibana without altering tsconfig.

@thompsongl thompsongl marked this pull request as ready for review July 9, 2020 15:21
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3715/

@chandlerprall
Copy link
Contributor

The generated files don't look correct, all of the declare module statements define @elastic/eui/lib/test but that makes the index.ts definition re-export from the non-existent definitions

declare module '@elastic/eui/lib/test' {
	export { requiredProps } from '@elastic/eui/lib/test/required_props';
	export { takeMountedSnapshot } from '@elastic/eui/lib/test/take_mounted_snapshot';
	export { findTestSubject } from '@elastic/eui/lib/test/find_test_subject';
	export { startThrowingReactWarnings, stopThrowingReactWarnings, } from '@elastic/eui/lib/test/react_warnings';
	export { sleep } from '@elastic/eui/lib/test/sleep';

}

@thompsongl
Copy link
Contributor Author

The generated files don't look correct

Good catch. Updated to include full module names.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3715/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! Ran build locally, confirmed enzyme and the test utilities are no longer mentioned in eui.d.ts and are now present in es/index.d.ts and lib/index.d.ts

@thompsongl
Copy link
Contributor Author

jenkins test this

"PUPPETEER_SKIP_CHROMIUM_DOWNLOAD"

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3715/

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

Successfully merging this pull request may close these issues.

Remove unexposed functions from declaration files
3 participants