Convert remaining `.js` files in `src/` to `.ts` by cee-chen · Pull Request #5044 · elastic/eui

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

 */

-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

Conversation

cee-chen commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What this PR does:

What this PR does not do:

QA

Checklist

Uh oh!

cee-chen Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thompsongl Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Aug 17, 2021

Uh oh!

thompsongl commented Aug 18, 2021

Uh oh!

kibanamachine commented Aug 18, 2021

Uh oh!

thompsongl commented Aug 18, 2021

Uh oh!

thompsongl commented Aug 18, 2021

Uh oh!

cee-chen commented Aug 18, 2021

Uh oh!

cee-chen commented Aug 18, 2021

Uh oh!

kibanamachine commented Aug 18, 2021

Uh oh!

thompsongl commented Aug 19, 2021

Uh oh!

cee-chen commented Aug 19, 2021

Uh oh!

thompsongl commented Aug 19, 2021

Uh oh!

cee-chen commented Aug 19, 2021

Uh oh!

thompsongl commented Aug 19, 2021

Uh oh!

cee-chen commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Aug 19, 2021

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Aug 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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