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

feat: add vega interpreter as dependency and support custom interpreter #747

Merged
merged 3 commits into from
Sep 17, 2021

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Sep 13, 2021

New version of #475

📦 Published PR as canary version: 6.18.3--canary.747.450ca4f.0

✨ Test out this PR locally via:

npm install vega-embed@6.18.3--canary.747.450ca4f.0
# or 
yarn add vega-embed@6.18.3--canary.747.450ca4f.0

@domoritz
Copy link
Member Author

@stefanw could you review this pull request?

@stefanw
Copy link

stefanw commented Sep 14, 2021

Setting the interpreter sets ast to true by default.

Currently, this doesn't seem to be included.

I'm unsure what level of abstraction vega-embed is going for. An idea could be offer an option noeval (or similar) which could then set ast and expr appropriately behind the scenes. This makes it easy to configure CSP-compliance without having to learn about how vega actually achieves it. Or rephrased: as a vega-embed user, do I care that vega needs to generate an AST and pass it to an expression interpreter?

@domoritz
Copy link
Member Author

domoritz commented Sep 14, 2021

Setting the interpreter sets ast to true by default.

Right, I removed that. I don't expect many people have to change the interpreter and if they set it, it doesn't mean they always want to use it (hence it shouldn't enable ast implicitly). I updated the comment.

Since expr is already set by default, setting ast is pretty much your proposed noeval. I agree that ast isn't at the right level of thinking for most users but since we already have the option, I can't remove it without making a breaking change.

We can reconsider the naming in the future but for now, I'll leave the names as they are. Hopefully, the documentation is good enough so that people find the option they need.

@hydrosquall hydrosquall changed the base branch from master to next September 16, 2021 23:55
@domoritz domoritz merged commit cbd54ba into next Sep 17, 2021
@domoritz domoritz deleted the dom/interpreter-2 branch September 17, 2021 01:45
@github-actions github-actions bot mentioned this pull request Sep 17, 2021
@github-actions
Copy link

🚀 PR was released in v6.19.0 🚀

@github-actions github-actions bot added released this feature has been released! and removed prerelease labels Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released this feature has been released!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants