Convert remaining `.js` files in `src/` to `.ts` #5044

cee-chen · 2021-08-17T22:17:36Z

Summary

closes #5034

What this PR does:

Converts src/components/index.js to .ts (9ce7d2b)
Converts src/index.js to .ts (98542a6)
Fixes the 2 typescript errors that arose from yarn build/tsc --noEmit -p tsconfig-builttypes.json (d8733a7)
Converts all index.ts exports from specific component exports to * wildcard exports (373e92c)

I leaned hard on Greg's previous PR #4695 for this work - thanks for doing the heavy lifting @thompsongl!

What this PR does not do:

Address https://github.com/elastic/eui/blob/master/scripts/dtsgenerator.js#L95-L99

QA

yarn build in EUI passed with no typescript errors
yarn kbn boostrap --no-validate in Kibana passed with no errors when pointed at the built tgz
yarn start in Kibana started with no errors, and styles/components loaded normally at a quick glance (including dark mode)
node scripts/type_check passed in Kibana with no errors

Checklist

Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] 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

- which involves adding an index.d.ts to packages for react datepicker

Credit to Greg for the original fixes

(in order to include/export types)

cee-chen · 2021-08-17T22:19:53Z

src/components/index.ts

@@ -6,328 +6,165 @@
 * Side Public License, v 1.
 */

-export { EuiAccordion } from './accordion';
+export * from './accordion';


I believe this src/components/index.ts change is required for Kibana/other apps to correctly import EUI type definitions - without this change, I was seeing the following types of errors during Kibana's yarn bootstrap step:

info [bazel] packages/kbn-securitysolution-autocomplete/src/field/index.tsx:10:23 - error TS2305: Module '"@elastic/eui"' has no exported member 'EuiComboBoxOptionOption'.
info [bazel]
info [bazel] 10 import { EuiComboBox, EuiComboBoxOptionOption } from '@elastic/eui';

I copied this export * solution from @thompsongl's previous PR in an attempt to fix the above error, and it appears to have done so, but I'd definitely appreciate a sanity check on whether both the problem and this solution are valid, or if we might run into unexpected errors as a result of over-exporting in the future. 🤔

Yes, we now need to export the types from this file and the * approach makes sense because the list of exports coming from the individual component index.ts files is curated.

cee-chen · 2021-08-17T22:23:10Z

👋 Hey Chandler and Greg! This PR is working for me locally in both EUI and Kibana, but I'm not super confident in my Typescript project/config-fu and would love someone to double check that my QA steps also pass for them locally. I've already hit my quota of breaking Kibana master this month, haha 😅

I could also use some guidance based on this PR what exactly from the checklist needs to be QA'd and what doesn't - in theory since this affects the entire project the answer could be "yes, everything", but in practice I'm not 100% sure. Additionally, does this PR require a CHANGELOG entry as well since it's in src/, or is it a weird gray area since it's tech debt and doesn't affect functionality?

kibanamachine · 2021-08-17T22:43:49Z

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

thompsongl · 2021-08-18T15:35:44Z

ERROR: Failed to set up Chromium r818858! Set "PUPPETEER_SKIP_DOWNLOAD" env variable to skip download.

jenkins test this

kibanamachine · 2021-08-18T15:59:21Z

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

thompsongl · 2021-08-18T16:24:56Z

I could also use some guidance based on this PR what exactly from the checklist needs to be QA'd and what doesn't

Yeah this falls into some gray area. I think the QA list you added is more appropriate than the standard Checklist. Probably still worth building and spot checking the docs, but the downstream/Kibana steps are more likely to catch bugs.
I wouldn't mind seeing a changelog entry for this, mostly so we easily reference when this change was made.

thompsongl · 2021-08-18T16:28:37Z

[ts refs] proc [tsc] x-pack/plugins/infra/public/components/logging/log_analysis_setup/missing_setup_privileges_tooltip.tsx:8:22 - error TS2305: Module '"@elastic/eui"' has no exported member 'PropsOf'.
[ts refs] proc [tsc]
[ts refs] proc [tsc] 8 import { EuiToolTip, PropsOf } from '@elastic/eui';

I built a tgz and ran bootstrap in Kibana, and got a few errors like above. Looks like we may also want to export * from src/components/common.ts?

cee-chen · 2021-08-18T16:38:20Z

I built a tgz and ran bootstrap in Kibana, and got a few errors like above. Looks like we may also want to export * from src/components/common.ts?

ARGH haha sorry, I saw that in your original PR but didn't see a common/ folder so I thought it was something that got removed at some point 🤦‍♀️ Total brainfart on my part, I'll add that. Also, how tf did my Kibana pass? ☠️

- I think it was caused by the common.ts export? Not totally sure why it only just started happening - PR is slowly just becoming Greg's old PR over time ha

cee-chen · 2021-08-18T17:45:09Z

OK, I think this new set of commits/changes should be ready to test again with yarn build and yarn kbn bootstrap --no-validate.

I did have to run yarn kbn clean before bootstrap (was getting some really weird errors about the test helpers not being available 😕) - although now I'm not sure if that's what's giving me false positives in Kibana when I should be seeing errors...?

kibanamachine · 2021-08-18T18:06:42Z

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

thompsongl · 2021-08-19T19:39:06Z

was getting some really weird errors about the test helpers not being available

[ts refs] proc [tsc] src/plugins/discover/public/application/angular/context/components/action_bar/action_bar.test.tsx:12:33 - error TS2307: Cannot find module '@elastic/eui/lib/test' or its corresponding type declarations.
[ts refs] proc [tsc]
[ts refs] proc [tsc] 12 import { findTestSubject } from '@elastic/eui/lib/test';

I'm still getting errors like above during bootstrap, after a clean

cee-chen · 2021-08-19T19:42:41Z

Hm, super bizarre (also not sure why my local Kibana isn't having issues). Do you have any idea how to fix that specific error Greg? I tried doing export * from './tests' in index.ts but I started getting a ton of EUI yarn build errors after adding that ☹️

thompsongl · 2021-08-19T20:09:42Z

Might be a false alarm on my part. Checking.

cee-chen · 2021-08-19T20:34:15Z

Just tried again with latest master kibana and latest master EUI merged into this branch and didn't get any errors on build or bootstrap 🤔 LMK if I can help or hop on a call!

thompsongl · 2021-08-19T21:16:15Z

☝️ Tracked the discrepancy down to using yarn pack vs npm pack when testing in Kibana.
lib/test/ was excluded from the package when using yarn, apparently because it respects .npmignore (??) which npm does not take in to account when building packages (??)

- Per Greg's eagle eyes, `yarn pack` was failing to bundle/include the `src/test/` folder somehow and causing test failures for plugins in Kibana trying to import `lib/test/` helpers - `npm pack` doesn't have this issue, so I opted to create a new command that enforces npm pack over yarn pack

cee-chen · 2021-08-19T21:21:58Z

27ccfd3 should address enforcing npm pack over yarn pack 🤦‍♀️ Thanks so much for figuring this out Greg! I can confirm that I tested again with a npm packed .tgz and build & bootstrap both ran correctly, and I was able to run yarn test:jest on a random Kibana test that imported an EUI test helper and have it pass with no failures (it would fail with import errors on the yarn packed version).

kibanamachine · 2021-08-19T21:46:50Z

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

thompsongl

LGTM 🥳

Built, checked packaged contents, checked eui.d.ts output, tested in Kibana.

cee-chen · 2021-08-19T22:14:28Z

Thanks a million Greg!! Merging here shortly

If something goes wrong on the Kibana uppage, feel free to blame me!

cee-chen added 4 commits August 17, 2021 13:42

Convert src/components/index.js to .ts

9ce7d2b

- which involves adding an index.d.ts to packages for react datepicker

Convert src/index.js to .ts

98542a6

Fix typescript errors

d8733a7

Credit to Greg for the original fixes

Change index.ts to export * instead of a specific list

373e92c

(in order to include/export types)

cee-chen requested review from chandlerprall and thompsongl August 17, 2021 22:17

cee-chen commented Aug 17, 2021

View reviewed changes

cee-chen added 4 commits August 18, 2021 09:43

Merge branch 'master' into src-js-to-ts

019e449

PR feedback: Fix missing common export

05a36ee

Add CHANGELOG entry

734a5cf

Fix new tsc error

1f872e4

- I think it was caused by the common.ts export? Not totally sure why it only just started happening - PR is slowly just becoming Greg's old PR over time ha

Merge branch 'master' into src-js-to-ts

b6aae35

thompsongl approved these changes Aug 19, 2021

View reviewed changes

cee-chen merged commit 5cde110 into elastic:master Aug 19, 2021

cee-chen deleted the src-js-to-ts branch August 19, 2021 22:16

cee-chen mentioned this pull request Sep 23, 2021

[EuiIcon] Convert generated icon .js assets to .tsx files #5212

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert remaining `.js` files in `src/` to `.ts` #5044

Convert remaining `.js` files in `src/` to `.ts` #5044

cee-chen commented Aug 17, 2021 •

edited

Loading

cee-chen Aug 17, 2021 •

edited

Loading

thompsongl Aug 18, 2021

cee-chen commented Aug 17, 2021 •

edited

Loading

kibanamachine commented Aug 17, 2021

thompsongl commented Aug 18, 2021

kibanamachine commented Aug 18, 2021

thompsongl commented Aug 18, 2021

thompsongl commented Aug 18, 2021

cee-chen commented Aug 18, 2021

cee-chen commented Aug 18, 2021

kibanamachine commented Aug 18, 2021

thompsongl commented Aug 19, 2021

cee-chen commented Aug 19, 2021

thompsongl commented Aug 19, 2021

cee-chen commented Aug 19, 2021

thompsongl commented Aug 19, 2021

cee-chen commented Aug 19, 2021 •

edited

Loading

kibanamachine commented Aug 19, 2021

thompsongl left a comment

cee-chen commented Aug 19, 2021

Convert remaining .js files in src/ to .ts #5044

Convert remaining .js files in src/ to .ts #5044

Conversation

cee-chen commented Aug 17, 2021 • edited Loading

Summary

What this PR does:

What this PR does not do:

QA

Checklist

cee-chen Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

thompsongl Aug 18, 2021

Choose a reason for hiding this comment

cee-chen commented Aug 17, 2021 • edited Loading

kibanamachine commented Aug 17, 2021

thompsongl commented Aug 18, 2021

kibanamachine commented Aug 18, 2021

thompsongl commented Aug 18, 2021

thompsongl commented Aug 18, 2021

cee-chen commented Aug 18, 2021

cee-chen commented Aug 18, 2021

kibanamachine commented Aug 18, 2021

thompsongl commented Aug 19, 2021

cee-chen commented Aug 19, 2021

thompsongl commented Aug 19, 2021

cee-chen commented Aug 19, 2021

thompsongl commented Aug 19, 2021

cee-chen commented Aug 19, 2021 • edited Loading

kibanamachine commented Aug 19, 2021

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Aug 19, 2021

Convert remaining `.js` files in `src/` to `.ts` #5044

Convert remaining `.js` files in `src/` to `.ts` #5044

cee-chen commented Aug 17, 2021 •

edited

Loading

cee-chen Aug 17, 2021 •

edited

Loading

cee-chen commented Aug 17, 2021 •

edited

Loading

cee-chen commented Aug 19, 2021 •

edited

Loading