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

TypeScriptify visualize loader #21025

Merged
merged 11 commits into from Jul 25, 2018

Conversation

Projects
None yet
5 participants
@timroes
Copy link
Contributor

commented Jul 20, 2018

This PR also refactores and improves a couple of minor issues:

  • We now throw an error in case we cannot find the request/response handler for a visualization instead of failing obscure.
  • Rename visualizeLoader to visualizeLoader.render
  • Remove a couple of unused services is VisualizeLoader
  • Actually check in courier request handler if timeRange is set before applying it as a filter.
  • When calling fetchAndRender multiple times we now force a fetch as long as a single one of those calls was a forceFetch call. Previously we always used the parameter passed to the last call before debounce was triggered.
  • Only use timeRange in date_histogram if it isn't undefined.
@elasticmachine

This comment has been minimized.

Copy link

commented Jul 20, 2018

timroes added some commits Jul 20, 2018

@timroes timroes force-pushed the timroes:ts-loaders branch from 65effe7 to 5b9548e Jul 23, 2018

@timroes timroes requested a review from weltenwort Jul 23, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 23, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 23, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 24, 2018

@timroes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 24, 2018

timroes added some commits Jul 24, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 24, 2018

@ppisljar

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

UI Functional Tests.test/functional/apps/dashboard/_embeddable_rendering·js.dashboard app using current data dashboard embeddable rendering adding visualizations (from (UI Functional Tests.xml))

i think the failiure is relevant, but just to be sure:

jenkins, test this

@weltenwort
Copy link
Contributor

left a comment

Quite a challenging undertaking, but aside from a few suggestions below it looks quite good to me.

@@ -17,8 +17,13 @@
* under the License.
*/

interface IInjector {

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

Have you considered using auto.IInjectorService from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a2a659c351b611398fd2f3f577fd545dab335306/types/angular/index.d.ts#L132 instead? The @types/angular package is already a dependency.

This comment has been minimized.

Copy link
@timroes

timroes Jul 24, 2018

Author Contributor

The auto namespace is not exported, so unfortunately we can't use that typing (I have no idea why the typing doesn't export that type, since it's a legit service to inject).

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

good point

const handler = loader.embedVisualizationWithSavedObject(newContainer()[0], createSavedObject(), {
timeRange: { from: 'now-7d', to: 'now' }
});

// Wait for the initial fetch and render to happen
await timeout(150);

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

I'm sure there is a reason for using these timeouts here, but I can't not comment on it. It looks like a flaky test in the making. 😉 Is there no way to synchronously force the "fetch and render to happen"? Maybe the time mocking functionality of sinon can be of use?

This comment has been minimized.

Copy link
@timroes

timroes Jul 24, 2018

Author Contributor

Unfortunately we currently cannot (I think I left a comment in the timeout below), since we use a too old lodash version. Basically we need to wait for the debounce to happen. Lodash prior 4, does not properly work together with jest or sinon fake timers, so that's the most reasonable way to trigger it imho. Also since we don't wait for something "arbitrary long" I don't think that will ever introduce flakyness, but we just need the debounce to be triggered. As soon as we update to lodash 4, we can use sinon.useFakeTimers() instead to make this sync.

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

How do you know that the debounce has always happened after 150ms? That looks suspiciously arbitrary. 😇 waiting for the "next tick" like in the prior test cases is not sufficient?

This comment has been minimized.

Copy link
@timroes

timroes Jul 24, 2018

Author Contributor

Unfortunately that's not enough, since in that case we actually need to wait for the debounce to be done, while the above doesn't care about that. After an update to lodash 4 that might be better, since we don't need to give it an arbitrary number here anymore, but can "flush" all pending debounce operations.

*/
import { isEqual } from 'lodash';

// @ts-ignore

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

How about adding stub .d.ts files with any exports for these registries and similar imports in this file to avoid masking other problems with blanket @ts-ignores?

queryFilter: QueryFilter;
}

export type RequestHandler = (vis: Vis, params: RequestHandlerParams) => any;

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

Would it be helpful to add a generic type argument for the return value?

private previousRequestHandlerResponse: any;

constructor(private readonly vis: Vis, Private: IPrivate) {
const { requestHandler, responseHandler } = vis.type;

This comment was marked as resolved.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

Would this be a good opportunity to add types for VisType.requestHandler and VisType.responseHandler in vis_type.d.ts?

*/

import chrome from '../../chrome';
// @ts-ignore implicit-any

This comment has been minimized.

Copy link
@weltenwort

weltenwort Jul 24, 2018

Contributor

How about adding that export to the type in the corresponding query_filter.d.ts?

timroes added some commits Jul 24, 2018

@timroes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Adressed all issues and merged master to include #20863

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 24, 2018

@ppisljar
Copy link
Member

left a comment

LGTM, tested on chrome linux

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 24, 2018

@timroes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

commented Jul 24, 2018

@timroes timroes merged commit 967cb4e into elastic:master Jul 25, 2018

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@timroes timroes deleted the timroes:ts-loaders branch Jul 25, 2018

timroes added a commit to timroes/kibana that referenced this pull request Jul 25, 2018

TypeScriptify visualize loader (elastic#21025)
* TypeScriptify Vis loaders

* Fix issue with undefined timeRange

* Fix chrome typing

* Fix unit tests

* Fix this issue

* Add missing uiState to request handler

* Implement Felix's suggestions

* Add timefilter listener

timroes added a commit that referenced this pull request Jul 25, 2018

TypeScriptify visualize loader (#21025) (#21190)
* TypeScriptify Vis loaders

* Fix issue with undefined timeRange

* Fix chrome typing

* Fix unit tests

* Fix this issue

* Add missing uiState to request handler

* Implement Felix's suggestions

* Add timefilter listener
@bhavyarm

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

How do we test this PR @timroes? Thanks!

@timroes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@bhavyarm Ideally this PR should not have changed any functionality, so it cannot be tested from a functional point. But all visualizations (everywhere) should still work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.