EuiModal, EuiConfirmModal: converted to Typescript #2742

ffknob · 2020-01-08T13:40:30Z

Summary

Converted EuiModal and EuiConfirmModal to Typescript.

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 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

elasticmachine · 2020-01-08T13:40:32Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

ffknob · 2020-01-08T13:43:28Z

I need some help with the test. By now I'm not getting pass this error here:

$ yarn test confirm_modal
yarn run v1.19.0
$ yarn lint && yarn test-unit modal
$ yarn tsc --noEmit && yarn lint-es && yarn lint-sass
$ /mnt/data/Desenvolvimento/workspace/eui/node_modules/.bin/tsc --noEmit
$ eslint --cache '{src,src-docs,src-framer}/**/*.{ts,tsx,js}' --max-warnings 0
$ sass-lint -v --max-warnings 0
$ cross-env NODE_ENV=test jest --config ./scripts/jest/config.json modal
 PASS  src/components/modal/modal_header.test.tsx
 PASS  src/components/modal/modal_body.test.tsx
 PASS  src/components/modal/modal_footer.test.tsx
 PASS  src/components/modal/modal_header_title.test.tsx
 PASS  src/components/modal/modal.test.tsx
 FAIL  src/components/modal/confirm_modal.test.tsx
  ● Test suite failed to run

    TypeError: Cannot read property 'type' of null

      1152 |   VariableDeclaration: node => {
      1153 |     return node.declarations.reduce((declarations, declaration) => {
    > 1154 |       if (declaration.init.type === 'ObjectExpression') {
           |                            ^
      1155 |         declarations.push({
      1156 |           name: declaration.id.name,
      1157 |           definition: declaration.init,

      at VariableDeclaration.node.declarations.reduce (scripts/babel/proptypes-from-ts-props/index.js:1154:28)
          at Array.reduce (<anonymous>)
      at Object.VariableDeclaration (scripts/babel/proptypes-from-ts-props/index.js:1153:30)
      at extractTypeDefinition (scripts/babel/proptypes-from-ts-props/index.js:1182:42)
      at PluginPass.visitProgram (scripts/babel/proptypes-from-ts-props/index.js:1354:15)
      at newFn (node_modules/@babel/traverse/lib/visitors.js:193:21)
      at NodePath._call (node_modules/@babel/traverse/lib/path/context.js:53:20)
      at NodePath.call (node_modules/@babel/traverse/lib/path/context.js:40:17)
      at NodePath.visit (node_modules/@babel/traverse/lib/path/context.js:88:12)
      at TraversalContext.visitQueue (node_modules/@babel/traverse/lib/context.js:118:16)

Snapshot Summary
 › 2 snapshot files obsolete from 2 test suites. To remove them all, run `yarn run test-unit -u`.

Test Suites: 1 failed, 5 passed, 6 total
Tests:       5 passed, 5 total
Snapshots:   2 files obsolete, 5 passed, 5 total
Time:        7.752s
Ran all test suites matching /modal/i.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Any clues? Perhaps something to do with my defined types... Other thig worth mentioning is that I had to yarn add @types/sinon in order to satisfy Typescript.

pugnascotia · 2020-01-08T14:15:21Z

When I have performed TypeScript conversions, I've often removed sinon in favour of Jest's built-in mock functions. How do you feel about doing that?

ffknob · 2020-01-08T14:20:49Z

When I have performed TypeScript conversions, I've often removed sinon in favour of Jest's built-in mock functions. How do you feel about doing that?

Hi @pugnascotia
Truth be told I have no clue on how to do that... but if you point me to an example test file I'll be more than happy to learn and implement this.

Does that mean that I should also remove the @types/sinon dependency for the whole project? (because as I understood from your answer it is not being used in TS components)

pugnascotia · 2020-01-08T14:25:46Z

It's pretty simple, see this example, but let me know if you need any help. There's also the Jest mock functions documentation.

You could then remove @types/sinon if it's not being used (i.e. no tests written in TypeScript are using it).

ffknob · 2020-01-08T14:39:03Z

It's pretty simple, see this example, but let me know if you need any help. There's also the Jest mock functions documentation.

You could then remove @types/sinon if it's not being used (i.e. no tests written in TypeScript are using it).

I think I managed to remove sinon and change it to Jest mock functions, but the above error persists...

pugnascotia · 2020-01-09T09:30:28Z

@ffknob the problem is in scripts/babel/proptypes-from-ts-props/index.js. I changed it as follows, in order to make it work:

@@ -1151,7 +1157,7 @@ const typeDefinitionExtractors = {
    */
   VariableDeclaration: node => {
     return node.declarations.reduce((declarations, declaration) => {
-      if (declaration.init.type === 'ObjectExpression') {
+      if (declaration.init != null && declaration.init.type === 'ObjectExpression') {
         declarations.push({
           name: declaration.id.name,
           definition: declaration.init,

@chandlerprall does the above change seem reasonable? The script seems to be failing on a line like:

let onConfirm: any;

… babel plugin; wrapped EuiConfirmModal tests in a describe block; changed two any types to jest.Mock

chandlerprall · 2020-01-10T19:27:32Z

Threw a pull request at @ffknob's branch - ffknob#3

It contains the empty variable declaration fix from @pugnascotia (thanks for debugging that, Rory!), updates the any types to jest.Mock, and wrapped the entire test suite in a single EuiConfirmModal describe block for best practice.

Couple updates for EuiConfirmModal PR

thompsongl

ffknob#4 will get this PR moving again. I had some local changes going, so I opened a PR instead of leaving detailed review comments.

Thanks for your patience, @ffknob!

TS clean up

ffknob · 2020-01-22T19:27:20Z

ffknob#4 will get this PR moving again. I had some local changes going, so I opened a PR instead of leaving detailed review comments.

Thanks for your patience, @ffknob!

thompsongl · 2020-01-22T19:37:43Z

Just need to rebase/merge master, I think

ffknob · 2020-01-22T19:39:04Z

Just need to rebase/merge master, I think

Just merged with master... hope I got it all right.

Thanks for the help!

thompsongl · 2020-01-22T19:43:44Z

jenkins test this

Converts EuiModal and EuiConfirmModal to Typescript

e1418fd

This was referenced Jan 8, 2020

Tipo: F / Cidade: <Porto Alegre - RS - BR> elastic/Elastic-Contributor-Program#435

Open

EuiModal needs to be converted to TS #2662

Closed

ffknob added 2 commits January 8, 2020 10:44

Adds PR number to CHANGELOG

23948f1

Merge branch 'master' into convert-EuiModal-to-TS

0d6c76d

pugnascotia added the typescript label Jan 8, 2020

Removes sinon from EuiConfirmModal tests

08a26b8

Created test and fix for empty variable declarations in ts->proptypes…

5931131

… babel plugin; wrapped EuiConfirmModal tests in a describe block; changed two any types to jest.Mock

ffknob and others added 2 commits January 10, 2020 17:30

Merge pull request #3 from chandlerprall/convert-EuiModal-to-TS

f0873c6

Couple updates for EuiConfirmModal PR

TS clean up

38366bf

thompsongl reviewed Jan 22, 2020

View reviewed changes

Merge pull request #4 from thompsongl/convert-EuiModal-to-TS

52a09bb

TS clean up

ffknob closed this Jan 22, 2020

Merge branch 'master' into convert-EuiModal-to-TS

a561863

ffknob reopened this Jan 22, 2020

thompsongl approved these changes Jan 22, 2020

View reviewed changes

thompsongl merged commit a630dd7 into elastic:master Jan 23, 2020

thompsongl mentioned this pull request Jan 28, 2020

Upgrade EUI to v18.3.0 elastic/kibana#56228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EuiModal, EuiConfirmModal: converted to Typescript #2742

EuiModal, EuiConfirmModal: converted to Typescript #2742

ffknob commented Jan 8, 2020

elasticmachine commented Jan 8, 2020

ffknob commented Jan 8, 2020

pugnascotia commented Jan 8, 2020

ffknob commented Jan 8, 2020

pugnascotia commented Jan 8, 2020

ffknob commented Jan 8, 2020

pugnascotia commented Jan 9, 2020

chandlerprall commented Jan 10, 2020 •

edited

Loading

thompsongl left a comment

ffknob commented Jan 22, 2020

thompsongl commented Jan 22, 2020

ffknob commented Jan 22, 2020

thompsongl commented Jan 22, 2020

EuiModal, EuiConfirmModal: converted to Typescript #2742

EuiModal, EuiConfirmModal: converted to Typescript #2742

Conversation

ffknob commented Jan 8, 2020

Summary

Checklist

elasticmachine commented Jan 8, 2020

ffknob commented Jan 8, 2020

pugnascotia commented Jan 8, 2020

ffknob commented Jan 8, 2020

pugnascotia commented Jan 8, 2020

ffknob commented Jan 8, 2020

pugnascotia commented Jan 9, 2020

chandlerprall commented Jan 10, 2020 • edited Loading

thompsongl left a comment

Choose a reason for hiding this comment

ffknob commented Jan 22, 2020

thompsongl commented Jan 22, 2020

ffknob commented Jan 22, 2020

thompsongl commented Jan 22, 2020

chandlerprall commented Jan 10, 2020 •

edited

Loading