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

Support vega-lite v5 #2717

Closed
ohltyler opened this issue Nov 1, 2022 · 1 comment · Fixed by #3076
Closed

Support vega-lite v5 #2717

ohltyler opened this issue Nov 1, 2022 · 1 comment · Fixed by #3076
Assignees
Labels
dependencies Pull requests that update a dependency file feature-anywhere unified visualization UX v2.5.0 'Issues and PRs related to version v2.5.0' visualizations Issues and PRs related to visualizations

Comments

@ohltyler
Copy link
Member

ohltyler commented Nov 1, 2022

Currently Dashboards has a hard dependency on vega-lite v4. v5 was released in early 2021 and provides a lot of functionality and simplicity improvements related to interactivity on the charts using parameters.

Upgrading will provide a lot of benefits:

  • allow users to render vega-lite specs with v5 schema in the vega plugin
  • simplify the implementation for annotation interaction functionality as part of the ongoing feature-anywhere projects
  • prevent wasted dev work of migrating any chart types with vega-lite, which will need to be re-done when moving to v5

I've tried to get a v5 version of vega-lite working in Dashboards, but have run into some issues:

To address this, I've tried to follow guidance here to use babel for transpiling the files into an ES5 bundle. I've played around with changing the webpack config for the osd-optimizer package to specifically transpile the vega-lite package before bundling:

  • commenting out the noParse arg here
  • excluding the vega-lite node_module here so it would use the default babel loader
  • adding a new entry specifically for vega-lite npm package, and trying different babel loaders (@babel/preset-env, the default BABEL_PRESET_PATH)

However, even when getting to transpile, it still seems to find non-ES5 code:

/node_modules/vega-lite/build/vega-lite.js:727
      deepMerge_(dest, s ?? {});
                          ^

SyntaxError: Unexpected token '?'

This seems to be a known issue - see vega/vega-lite#7759, vega/vega-lite#7595 (comment)

From the comment here it should be possible to configure the bundler to point to a correct target that will work, but I was unable to resolve the nullish operator error shown above.

@joshuarrrr
Copy link
Member

I'm still working on this but wanted to summarize some of what I've found so far.

Actual vega-lite usage

One important thing to note is that our dependency on vega-lite is very specific and targeted within the vis_type_vega plugin (the only plugin which uses it). We only need to import { compile, version} for use in src/plugins/vis_type_vega/public/data_model/vega_parser.ts. compile is used to compile the vega-lite specification down to a vega specification. Everything after that (including all rendering) is handled by vega, where we're already on the latest major version. version is just a static property for fetching the vega-lite version, and we also use it in src/plugins/vis_type_vega/public/vega_view/vega_base_view.js just as a debugging value.

One of the weird things about the vis_type_vega plugin is that it aliases both vega and vega-lite dependencies in src/plugins/vis_type_vega/public/lib/vega.js, which appears to be an unnecessary antipattern. I think it may have been a workaround to the issues we're facing now, where they decided to just use the built es5 modules rather than bundle them like normal dependencies. One reason it's an anitpattern is that you lose the typing info, and in fact there are some type mismatches and errors in the current implementation.

Preferred migration path

We consider direct dependencies in the OpenSearch Dashboards package.json to be part of the de facto public API subject to semver because of the way the shared dependency model for plugins works. So even though it's unlikely that many plugins are directly depending on "vega-lite": "^4.16.8", we can't properly bump it until a major version release. Instead, I propose that we add the v5 dependency with an alias: "vega-lite-next": "npm:vega-lite@5.6.0",

We can then freely update the vis_type_vega plugin to depend on vega-lite-next, because the compiler will still support older vega-lite specs.

Understanding of the problem so far

OpenSearch dashboards has a bit of a bespoke bundling process. Most bundling is handled by the osd-optimizer package, which uses webpack to actually build the bundles. One quirk of this setup is that many es6 syntax is really only supported by our bundler in typescript files, because the typescript compiler is responsible (and able) to parse and compile them correctly. Because we prefer typescript, we don't usually have es6 syntax in javascript files.

But vega-lite v5 does have some dependencies that have es6 features (like the nullish coalescing operator) in .js files. These don't get handled by the typescript compiler, and then webpack trips over the syntax because it's not supported in the version of acorn (which webpack uses to parse js) in webpack version 4. It appears that we may be able to fix this behavior simply by using yarn resolutions to bump the acorn version, but I haven't got it working quite yet. I'd prefer to avoid a webpack major version bump for this particular issue unless it's really necessary.

Other things investigated

Note that we do already require the necessary babel plugins for these operators:

// Optional Chaining proposal is stage 3 (https://github.com/tc39/proposal-optional-chaining)
// Need this since we are using TypeScript 3.7+
require.resolve('@babel/plugin-proposal-optional-chaining'),
// Nullish coalescing proposal is stage 3 (https://github.com/tc39/proposal-nullish-coalescing)
// Need this since we are using TypeScript 3.7+
require.resolve('@babel/plugin-proposal-nullish-coalescing-operator'),

That preset is included by the webpack_preset

Which is used by the webpack config in osd-optimizer:

{
test: /\.(js|tsx?)$/,
exclude: /node_modules/,
use: {
loader: 'babel-loader',
options: {
babelrc: false,
envName: worker.dist ? 'production' : 'development',
presets: [BABEL_PRESET_PATH],
},
},
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file feature-anywhere unified visualization UX v2.5.0 'Issues and PRs related to version v2.5.0' visualizations Issues and PRs related to visualizations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants