Convert services to TypeScript, part 2 #1392
Conversation
pugnascotia
force-pushed
the
convert-services-to-ts-pt2
branch
from
8d51935
to
007fd4b
Compare
| @@ -377,8 +447,6 @@ function getCrossAxisPosition({ | |||
| let crossAxisArrowPosition; | |||
| if (arrowConfig) { | |||
| const { arrowWidth } = arrowConfig; | |||
| const anchorSizeOnCrossAxis = anchorBoundingBox[crossAxisDimension]; | |||
| const anchorHalfSize = anchorSizeOnCrossAxis / 2; | |||
There was a problem hiding this comment.
AFAICT these lines were recalculating some values for no reason.
| componentDidMount() { | ||
| this.addEvent(this.props); | ||
| } | ||
|
|
||
| componentDidUpdate(prevProps) { | ||
| componentDidUpdate(prevProps: Props<E>) { |
There was a problem hiding this comment.
This isn't actually true, since the props could change to that we're handing a different event type. The types here are intended to ensure that the event type in the callback matches the event name, but that ends up leaking into the class' generic type. If the component instance receives different props, it can end up wrapping a different event type. In practice this may not be an issue?
|
@chandlerprall the build is breaking - would you mind taking a look? I added a change to this PR so that CI runs the build (it should have been doing that anyway), so the CI logs will show what broke. Seems to be duplicate declarations in |
|
@pugnascotia Just a heads up. Chandler is out for break. The good news is we have a new engineer joining the EUI team starting in January who can give us some more coverage here. If you're in a hury, you might want to ping one of the Cloud or Kibana devs to give you a proper review since this one is a bit too intense for Caroline or I :) That popover stuff can be difficult so it might just be best to wait till Jan on this one. |
|
What a slacker! 😁 |
| done(); | ||
| }, 8); | ||
| }); | ||
| }); | ||
|
|
||
| describe('pause', () => { | ||
| test('stops timer', done => { | ||
| const callbackSpy = sinon.spy(); | ||
| const callbackSpy = jest.fn(); |
There was a problem hiding this comment.
Thanks for converting these to jest's mock functions!
There was a problem hiding this comment.
This looks fantastic @pugnascotia , thank you! I'm looking into the issue with exported types.
pugnascotia
force-pushed
the
convert-services-to-ts-pt2
branch
from
eb9fb74
to
4f598bd
Compare
|
Thanks for the fix @chandlerprall, I've rebased on the laster master so this is good to review. |
Summary
Follow-up to #1360. Convert the remaining half of services to TypeScript. Notes:
proptypes-from-ts-propsBabel plugin to work around index signatures in interfaces.QueryandAstfromservicesto thecomponentstree. As far as I can tell, the export from services is hangover from a reverted attempt to move some of the query code to services, and it was preventing the move ofservices/index.jsto TS.Checklist