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

Embeddable API V2: Infrastructure changes necessary to support actions #35622

Closed

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Apr 25, 2019

Summary

No actual functionality changes should occur because of this PR, but much of the infrastructure changed in order to support dynamic actions, as well as embeddables existing outside the context of the dashboard application itself.

  • Dashboard viewport code (dashboard minus the top nav, filter/query bar and time picker) has been moved to a new plugin called dashboard_embeddable.
  • Embeddable API code moved to a new plugin called embeddable_api and no longer resides in ui/public.
  • Dashboard app code has all redux removed from it, no redux was copied over to the new dashboard_embeddable code.
  • dashboard_app code refactored and migrated to typescript. Changes in here:
    • Hook up communication between DashboardStateManager and DashboardContainer, so url changes propagate to the container and vice versa.

The most fragile parts of the code are in dashboard_app_controller and dashboard_state_manager. It's very difficult to jest test these angular -> react interactions so we have to rely on selenium tests for this coverage. A good next step would be to move the filter and query bar as components of the DashboardContainer. This would make the interactions much easier to jest test and they would be much more stable (and of course, much less angular glue code!).

TODOs

  • Solve bug where if factory doesn't exist you can't remove the embeddable from a dashboard. Write a jest test for this scenario.
  • Get all selenium tests passing
  • Remove renderInPanel from API, provide as optional react component wrapper only.

Related

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

Documentation added as a README.md in the embeddable_api folder.

TODO: post final coverage numbers here...

For maintainers

This could potentially break any developers currently using the Embeddable API v1 though it's unlikely it's being used externally as it was never advertised. We always knew it would be volatile.

@stacey-gammon stacey-gammon force-pushed the 2019-04-25-embeddable-api branch 5 times, most recently from 1ce9f30 to 6487d39 Compare April 26, 2019 16:29
@stacey-gammon stacey-gammon changed the title 2019 04 25 embeddable api [skip ci] 2019 04 25 embeddable api Apr 26, 2019
@stacey-gammon stacey-gammon force-pushed the 2019-04-25-embeddable-api branch 3 times, most recently from 77dcce7 to 77958a3 Compare April 28, 2019 18:43
@flash1293
Copy link
Contributor

flash1293 commented Apr 29, 2019

I looked through the types and noticed a few things:

  • The bug with the observable member in embeddable and strict function type checking goes away if instances of IEmbeddable instead of Embeddable are passed around (I started this a bit here: flash1293@1e24e4b ). This happens because there is no observable member in IEmbeddable and this implementation detail is hidden which IMHO makes sense even without the bug.
  • Try to use type inference in as many places as possible to avoid explicitly stating the complicated generic types, especially in the consumers of the embeddable api
  • Names of generic types: There are multiple conventions for this, for complicated concepts as in this case I like the T* naming, e.g. TEmbeddableInput instead of EI. I suggest to avoid naming them like classes as in src/legacy/core_plugins/dashboard_embeddable/public/embeddable/panel/create_panel_state.ts (PartialEmbeddableInput)
  • The panel state seems more like an object than a { [key: string]: unknown }, right? If the problem is key iteration and implicit any types, typing Object.keys like this Change type definition of Object.keys and for..in loop microsoft/TypeScript#24243 (comment) is probably an easier way.
  • The type EmbeddableInputMissingFromContainer is gone - is this the one we've been talking about with the never types to make sure nothing is overwritten with the wrong type?

@stacey-gammon stacey-gammon force-pushed the 2019-04-25-embeddable-api branch 9 times, most recently from 9a8d4a1 to 7f6f495 Compare April 30, 2019 14:19
@stacey-gammon stacey-gammon changed the title [skip ci] 2019 04 25 embeddable api Embeddable API V2: Infrastructure changes necessary to support actions Apr 30, 2019
@stacey-gammon stacey-gammon force-pushed the 2019-04-25-embeddable-api branch from 7f6f495 to 724247f Compare April 30, 2019 17:02
@stacey-gammon stacey-gammon force-pushed the 2019-04-25-embeddable-api branch 2 times, most recently from 945e782 to 1a009a4 Compare May 1, 2019 12:29
@elastic elastic deleted a comment from elasticmachine May 1, 2019
@elastic elastic deleted a comment from elasticmachine May 1, 2019
@elastic elastic deleted a comment from elasticmachine May 1, 2019
@elastic elastic deleted a comment from elasticmachine May 1, 2019
@elastic elastic deleted a comment from elasticmachine May 1, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I did some quick looking over of just the .scss files and has some questions/suggestions.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor Author

This PR is just too massive, so I'm breaking apart. So far I've got:

Next step I am thinking of is adding the entire dashboard_embeddable plugin over, just like I did with the Embeddable API plugin, and use it but only in the Embeddable Explorer sample plugin.

Then the final PR would be actually switching everything over to using the new embeddables.

@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

Formalize Embeddable Layer with tests and sample plugins Embeddables API
7 participants