-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Introduce Javascript controls #4076
Conversation
I have 2 concerns:
|
@fabianmenges thanks for the feedback. I agree that we should be extremely careful in what is exposed here, whatever goes in and out has to be treated like a public API, so no backwards incompatible changes should be allowed without release notes (if ever). We do control exactly what is accessible in the sandbox. I pass the After thinking this through I'm removing |
So this PR adds the new control but the control is not used anywhere. I'd like to merge this independently from the use cases which will come in future PRs in the geospatial space. Anytime this control will be used, we should consider the fact that it locks whatever it hooks (input and output) from backwards incompatible changes. |
This allows power-users to perform intricate transformations on data and objects using javascript code. The operations allowed are "sanboxed" or limited using node's vm `runInNewContext` https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options For now I'm only enabling in the line chart visualization, but the plan would be to go towards offering more power to people who can write some JS moving forward.
@fabianmenges mind taking another look? Here the control is not actually being used by any visualization (planning to use it on DECK.GL viz eventually), and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Introduce Javascript controls This allows power-users to perform intricate transformations on data and objects using javascript code. The operations allowed are "sanboxed" or limited using node's vm `runInNewContext` https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options For now I'm only enabling in the line chart visualization, but the plan would be to go towards offering more power to people who can write some JS moving forward. * Not applied
* Introduce Javascript controls This allows power-users to perform intricate transformations on data and objects using javascript code. The operations allowed are "sanboxed" or limited using node's vm `runInNewContext` https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options For now I'm only enabling in the line chart visualization, but the plan would be to go towards offering more power to people who can write some JS moving forward. * Not applied
This allows power-users to perform intricate transformations on data and
objects using javascript code.
The operations allowed are "sanboxed" or limited using node's vm
runInNewContext
https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options
For now I'm only enabling in the line chart visualization, but the plan
would be to go towards offering more power to people who can write some
JS moving forward.