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

Fix: correctly access types from "to" function #32691

Merged
merged 2 commits into from
Mar 12, 2019

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Mar 7, 2019

Closes #32597
Blocked by #32600 (mostly for verification reasons) merged

Types were being pulled from the handlers object, but they are not exposed this way anymore. They must be imported from @kbn/interpreter now.

This PR also makes the to function a browser-only function. Common functions were being registered in both places, but thanks to #33039, it was only ever run on the server, which there is no longer any need for.

This PR makes the following expression demodata | to type="null" go from this:

screenshot 2019-03-07 12 56 38

To this:

screenshot 2019-03-07 12 55 32


To test, you'll need to need to merge/rebase with #32600, otherwise you're going to see other error messages (specifically, "See server logs for details" and "Function to did not return anything.").

@w33ble w33ble added review v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v6.7.0 v7.2.0 labels Mar 7, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@w33ble w33ble requested a review from a team as a code owner March 7, 2019 20:02
@w33ble
Copy link
Contributor Author

w33ble commented Mar 7, 2019

@chrisdavies it occurred to me that we may need to check the environment to get the right registry, since we (I think incorrectly) have two different ones. I think without this, we'd end up bundling the wrong registry in the browser...

diff --git a/x-pack/plugins/canvas/common/functions/to.js b/x-pack/plugins/canvas/common/functions/to.js
index 9344265f59..cf47606a6a 100644
--- a/x-pack/plugins/canvas/common/functions/to.js
+++ b/x-pack/plugins/canvas/common/functions/to.js
@@ -5,7 +5,8 @@
  */
 
 import { castProvider } from '@kbn/interpreter/common';
-import { registries } from '@kbn/interpreter/server';
+import { registries as serverRegistries } from '@kbn/interpreter/server';
+import { registries as browserRegistries } from '@kbn/interpreter/public';
 
 export const to = () => ({
   name: 'to',
@@ -20,11 +21,13 @@ export const to = () => ({
       multi: true,
     },
   },
-  fn: (context, args) => {
+  fn: (context, args, { environment }) => {
     if (!args.type) {
       throw new Error('Must specify a casting type');
     }
 
+    const registries = environment === 'client' ? browserRegistries : serverRegistries;
+
     return castProvider(registries.types.toJS())(context, args.type);
   },
 });

Thoughts?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

LGTM

they are not exposed on the handlers object, they must be imported from @kbn/interpreter
@w33ble
Copy link
Contributor Author

w33ble commented Mar 12, 2019

OK, no need to handle the browser registries stuff, the server function is always used thanks to #33039.

@w33ble w33ble force-pushed the fix/to-function-types branch from 7925cbe to dd912c5 Compare March 12, 2019 20:30
types can be accessed in the browser now, and a common function makes no sense right now. also elastic#33039 prevents having the same function in both places anyway
@w33ble w33ble force-pushed the fix/to-function-types branch from dd912c5 to 213d510 Compare March 12, 2019 20:32
@w33ble
Copy link
Contributor Author

w33ble commented Mar 12, 2019

I've updated the PR to make the to function only run in the browser, seemed like that made the most sense and was the least likely to break in the future (ie. once #33039 is fixed).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@w33ble w33ble merged commit 7c7bcdd into elastic:master Mar 12, 2019
w33ble added a commit to w33ble/kibana that referenced this pull request Mar 12, 2019
* fix: correctly access types

they are not exposed on the handlers object, they must be imported from @kbn/interpreter

* chore: move to function to browser

types can be accessed in the browser now, and a common function makes no sense right now. also elastic#33039 prevents having the same function in both places anyway
w33ble added a commit to w33ble/kibana that referenced this pull request Mar 12, 2019
* fix: correctly access types

they are not exposed on the handlers object, they must be imported from @kbn/interpreter

* chore: move to function to browser

types can be accessed in the browser now, and a common function makes no sense right now. also elastic#33039 prevents having the same function in both places anyway
w33ble added a commit to w33ble/kibana that referenced this pull request Mar 12, 2019
* fix: correctly access types

they are not exposed on the handlers object, they must be imported from @kbn/interpreter

* chore: move to function to browser

types can be accessed in the browser now, and a common function makes no sense right now. also elastic#33039 prevents having the same function in both places anyway
w33ble added a commit that referenced this pull request Mar 12, 2019
* fix: correctly access types

they are not exposed on the handlers object, they must be imported from @kbn/interpreter

* chore: move to function to browser

types can be accessed in the browser now, and a common function makes no sense right now. also #33039 prevents having the same function in both places anyway
w33ble added a commit that referenced this pull request Mar 12, 2019
* fix: correctly access types

they are not exposed on the handlers object, they must be imported from @kbn/interpreter

* chore: move to function to browser

types can be accessed in the browser now, and a common function makes no sense right now. also #33039 prevents having the same function in both places anyway
w33ble added a commit that referenced this pull request Mar 12, 2019
* fix: correctly access types

they are not exposed on the handlers object, they must be imported from @kbn/interpreter

* chore: move to function to browser

types can be accessed in the browser now, and a common function makes no sense right now. also #33039 prevents having the same function in both places anyway
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.7.0 v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants