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

Introduce TypeScript support into the dev & build systems #1317

Merged
merged 64 commits into from
Nov 30, 2018

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Nov 16, 2018

Summary

closes #1262

  • TypeScript support within the EUI codebase (src, src-docs, and tests).
  • Prop definitions for React components are transpiled to React propTypes with a custom babel plugin
  • Spacer component, its test, and docs have been converted
  • common, required_props, and the predicates service have been converted

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs

  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@pugnascotia
Copy link
Contributor

Is there a plan for migrating components to TS? Will you / we / someone just work through them in a series of PRs? I can help if you like.

@chandlerprall
Copy link
Contributor Author

Have these changes been tested against Kibana?

I've tested them briefly in Kibana, doing more today & tomorrow.

Hmm, I'm now wondering if EUI is not very amenable to linking. Same issue when I switch to master.

I'm not sure what happened to make linking unhappy (it's something about the nested-nested node_modules in EUI), but I've started following this pattern:

  • in eui: npm pack creates a tarball in working dir with the files that would be published
  • extract tarball somewhere else (I do my home directory), will extract to a directory named package
  • yarn install ~/package (or whatever your path to the extracted package dir is
  • yarn has now installed the eui package as it would have been installed, just from a directory instead of npm

Is there a plan for migrating components to TS? Will you / we / someone just work through them in a series of PRs? I can help if you like.

I'm putting together a screencast walkthrough of the EuiSpacer conversion to help bring people up to speed. The plan is then a series of PRs - I was planning on putting a script together that can list the next candidates for conversion as we have to do a bottom-up approach to avoid importing a JS file into TS.

@jasonrhodes
Copy link
Member

  • extract tarball somewhere else (I do my home directory), will extract to a directory named package

FWIW, this npm pack process only worked for me when I did yarn install PATH and PATH pointed at the tarball created by npm pack directly, without extracting it. Which also was one less step!

Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

Added comments look good.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

This is fantastic. I tested everything by converting a component in TS. chandlerprall#7

Only other comment I'd make other than the ones below is that we should update the Yeoman scripts to build TSX files and docs for new components. If that's time consuming I'm OK merging this now and making it a separate PR.

src-docs/src/views/spacer/spacer.tsx Show resolved Hide resolved
className,
size,
...rest
}) => {
const classes = classNames(
'euiSpacer',
sizeToClassNameMap[size],
size ? sizeToClassNameMap[size] : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should cement this as a pattern. Your vid had you redeclaring the default prop value, which I'm sure people might do since it's clean, but let's make sure we do the ternary check in review so we don't have dupe values.

@chandlerprall
Copy link
Contributor Author

@jasonrhodes @pugnascotia @snide

Thank you all for your help on this; I've pushed up the final requested changes (yeoman template update, small docs codegen change). I'll merge this Friday morning Eastern Time unless anything else is mentioned.

@snide
Copy link
Contributor

snide commented Nov 29, 2018

Cool. I'll give the new stuff a check.

@chandlerprall
Copy link
Contributor Author

jenkins test this

1 similar comment
@chandlerprall
Copy link
Contributor Author

jenkins test this

@chandlerprall chandlerprall merged commit 5b2eb96 into elastic:master Nov 30, 2018
@chandlerprall chandlerprall deleted the feature/typescript branch November 30, 2018 16:38
@snide snide mentioned this pull request Nov 30, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TypeScript code within EUI
4 participants