Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jan 24, 2022

Summary

closes #5552

Upgrades @typescript-eslint to latest, adds @typescript-eslint/consistent-type-exports rule, and runs --fix on src/ exports

This converts our type exports to use export type instead of just plain export, but does not do any extra work around removing proptypes-from-ts-props or updating our compile scripts - I was hoping @thompsongl could carry the PR the rest of the way 🤞

proptypes-from-ts-props remains, but has been modified to remove 1) the configuration to prevent propType generation, and 2) the exit method to strip type exports from es build (handled elsewhere by babel now).

QA

  • Test final build against Kibana

@cee-chen cee-chen requested a review from thompsongl January 24, 2022 17:57
@cee-chen cee-chen added skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt labels Jan 24, 2022
@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

🎉 Confirmed that type exports are stripped from the es builds, most notably the optimize build without our custom babel plugin. Going to do some more testing around this to see if we can get further time improvements or config simplifications.

@thompsongl
Copy link
Contributor

thompsongl commented Jan 27, 2022

Going to do some more testing around this to see if we can get further time improvements or config simplifications.

I think the only thing we need to do is adjust the optimize babel config after this branch gets updated with main. We could probably asses the exit program in the proptypes-from-ts-types to see if it's still needed.

@cee-chen
Copy link
Contributor Author

Sweet! Just to check, do you want me to keep going with this PR or do you want to take it over?

@thompsongl
Copy link
Contributor

I can push some changes at the very least 👍

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

});
});

describe('remove types from exports', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is all handled by babel now outside of our custom plugin 🎉
The redundant code in the plugin has also been removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it turns out, I'll also need these tests removed to upgrade Typescript to 4.5.x (it starts outputting export {} instead of a blank string once upgraded 🤷‍♀️), so glad that we jumped on this tech debt item sooner rather than later!

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 1, 2022

Nice!! Just so I'm on the same page - we still need proptypes-from-ts-props for our docs, correct? It's only the generatePropTypes: false config that we no longer need?

@thompsongl
Copy link
Contributor

It's only the generatePropTypes: false config that we no longer need?

Correct. That and the function that used to remove type exports.

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 1, 2022

Do you think this is ready to move out of draft in that case once CI/tests pass?

@thompsongl thompsongl marked this pull request as ready for review February 1, 2022 21:29
@kibanamachine
Copy link

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

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.

Wooo!

✅ Code deletion
✅ More explicit organization
✅ Aligning with community standards

Tested:

  • yarn build and spot-checked some files in es and lib to ensure types are still omitted
  • yarn start and compared Props tabs between local & published docs
  • verified proptypes are still generated in the build output as expected
  • verified eslint complains about non-type type exports, and that --fix solves

@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 2, 2022

Wahoo!

@cee-chen cee-chen merged commit 331e6f6 into elastic:main Feb 2, 2022
@cee-chen cee-chen deleted the export-types branch February 2, 2022 21:01
@cee-chen cee-chen mentioned this pull request Feb 2, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use export type syntax and update es build

4 participants