vega / vega-lite-api

A JavaScript API for Vega-Lite.
https://observablehq.com/@vega/vega-lite-api
BSD 3-Clause "New" or "Revised" License
211 stars 17 forks source link

Avoid browser global conflict with vega-lite #24

Closed curran closed 5 years ago

curran commented 5 years ago

Currently, both vega-lite-api and vega-lite browser builds output the same global variable, namely vl. This is unfortunate, since it makes it difficult to work with vega-lite-api without using a build step or using a module loader like d3-require. To work around this conflict, I did the following:

<script src="https://unpkg.com/vega-lite@4.0.0-beta.0/build/vega-lite.min.js"></script>
<script>
  // Workadound for conflicting global.
  const vegalite = vl;
</script>
<script src="https://unpkg.com/vega-lite-api@0.1.0/build/vega-lite-api.min.js"></script>

Related:

The only change required would be this Rollup configuration here: https://github.com/vega/vega-lite-api/blob/master/rollup.js#L39

Suggested name: vlAPI.

jheer commented 5 years ago

Thanks for raising the issue! We should certainly avoid conflicting globals.

As vega-lite is typically called only sparingly (e.g., to compile a new spec), I wonder if renaming the default output to vegalite for VL 4.0 makes sense? (Vega made a similar vg -> vega change when moving from v2 to v3+.)

The VL API could then continue to use the more concise vl, which would be helpful given how often the API object can be referenced. One could of course re-bind variable names after import but it would be nice to not have that be a standard pattern. I'll let @kanitw, @domoritz, and @arvind chime in with their thoughts on this.

If not, I think vlAPI or perhaps the less "shouty" vlapi makes sense. Or maybe consider an alternative shorthand, such as vla or va (though I'm less enamored of those).

curran commented 5 years ago

I do like the idea of vl being the global for vega-lite-api and vegalite being the global for vega-lite. One thing to consider, though, is that vega-lite-api is so fresh that the global name is quite malleable. Probably nothing would break if it changes. However with vega-lite, there may be many sites that depend on the name of the global. These sites would all break at 4.0. In theory this is fine as it's a major version bump, but still it's breakage that could be avoided. Also, a change in vega-lite-api's global could happen more quickly, without waiting for vega-lite to be ready for 4.0.

Or how about just v or V for vega-lite-api? FWIW Leaflet's global is L.

arvind commented 5 years ago

In practice, I think the compile method is perhaps the only API folks ever interact with in the core Vega-Lite library. So switching its namespace to vegalite and giving vl over to the Vega-Lite JS API makes sense to me.

V feels too general to me -- does it refer to Vega, Vega-Lite (or a future Vega-Nano, vega/vega-lite#578)?

domoritz commented 5 years ago

I don't think we need to optimize for writability. Therefore, if be okay either among Vega-Lite's export to be vegalite but I don't know how good it would be to use vl for Vega-Lite API. My vote is for vlapi.

kanitw commented 5 years ago

but I don't know how good it would be to use vl for Vega-Lite API. My vote is for vlapi.

I prefer vl to vlapi, which is a bit too verbose to be called multiple times. For example, consider how many times we have to call vl in the code from this example:

vl.data(data)
  .transform(
    vl.window(vl.row_number().as('series')), // add index for each series
    vl.flatten('data'), // flatten each series array to individual data points
    vl.groupby('series').window(vl.row_number().as('index')) // add index for each point
  )
  .mark({type: 'line', fill: 'white', stroke: 'black', strokeWidth: 1})
  .encode(
    vl.x().fieldQ('index').axis(null),
    vl.y().fieldQ('data').scale({range: [step, -overlap * step]}).axis(null),
    vl.row().fieldN('series').header({title: null, labelPadding: 0, labelFontSize: 0})
  )
  .width(width)    // facet cell width
  .height(step)    // facet cell height
  .bounds('flush') // use facet cell height for layout, not the full mark bounds 
  .spacing(0)      // zero spacing between facet cells
  .padding(0)      // no chart padding
  .config({view: {stroke: null}}) // no cell borders
  .render()
domoritz commented 5 years ago

Hmm, I see. One (maybe not super important) concern I have with using vegalite is that people will start spelling Vega-Lite as VegaLite πŸ€ͺ.

Okay, so then let's do

vega for Vega (used to be vg) vegalite for Vega-Lite (currently vl) vl for the Vega-Lite API embed for Vega Embed (currently vegaEmbed) tooltip for Vega Tooltip (currently vegaTooltip) themes for Vega Themes (currently vegaThemes)

or

vega for Vega (used to be vg) vegaLite for Vega-Lite (currently vl) vl for the Vega-Lite API vegaEmbed for Vega Embed vegaTooltip for Vega Tooltip vegaThemes for Vega Themes

jheer commented 5 years ago

I like the second set better (vegaLite, vegaEmbed). More consistency, clearer names overall, less churn across packages, and perhaps the camel-case will better convey "Vega-Lite" rather than "vegalite". 😸

curran commented 5 years ago

I must say, this is the first time in my life I have ever seen that cat emoji. :smile_cat:

In that second list, vl stands out as not having the vega prefix.

What if the global were verbose, like vegaLiteApi, and folks could make an alias shorthand themselves, like const vl = vegaLiteApi;.

arvind commented 5 years ago

I would be fine introducing a minor inconsistency with the Vega-Lite API namespace. My concern with aliasing is that it introduces just a slight (arguably unnecessary) amount of friction into the process: e.g., copying and pasting a block of Vega-Lite JS code may not work right away; newcomers to JavaScript may not think to create the alias and then are saddled with the verbosity issues identified above.

domoritz commented 5 years ago

Let's go with the second set πŸˆπŸ˜ΊπŸ…πŸ†πŸ§ΆπŸΎπŸ˜ΌπŸ˜»πŸ¦

domoritz commented 5 years ago

Fixed in https://github.com/vega/vega-lite/pull/5343

curran commented 5 years ago

Hooray!