Deploying vega with    Cloudflare Pages

View logs

Thanks. I added a more generic comparator so we don't need two methods and added docs as well.

That's great, thank you! Having just one method is much preferred.

Hey @joelostblom, thanks for putting this together!

Using the branch preview ( https://sort-functions.vega-628.pages.dev/#/ ), could you link to a sample Vega or Vega-Lite definition that would make use of this new expression? We probably want at least 1 case each for each happy path (numbers, strings, maybe one with missing data). I'm not sure we need to have an especially "nice" behavior for the mixed datatype case other than to not crash.

what tests are needed,

I looked to see how tests in this package vega-interpreter are written ( https://github.com/vega/vega/tree/main/packages/vega-interpreter/test ), and found that it appears to involve adding an additional JSON file each to to https://github.com/vega/vega/tree/main/packages/vega/test/specs-valid and https://github.com/vega/vega/tree/main/packages/vega/test/scenegraphs . Once we confirm that this expression does what you need in the preview deploy, we can copy that JSON over to the test suite.

Sorry for the delay in getting back to you @hydrosquall . I was trying out the branch preview editor right now, and it seems like it doesn't recognize the sort function. Try replacing reverse with sort on line 15 in the following spec and you will see that it errors with Unrecognized function: sort. Open the Chart in the Vega Editor

I tried this spec btw. It definitely worked for me before but I also get Unrecognized function: sort right now.

{
  "$schema": "https://vega.github.io/schema/vega/v5.json",


  "signals": [
    {
      "name": "foo",
      "update": "sort([1,2,3])"
    }
  ]
}

From grepping the code for “unrecognized”, I think we might have to add
this new sort function to Vega-expression too.

Hmm, doesn't that sound odd given that other array functions such as reverse are not listed there?

Give that @domoritz mentioned the spec worked for him before, maybe there is something going on with the deploy preview?

I couldn't get it to work locally either yesterday. Need to dig a bit more what I did before

Yes, but reverse works so I'm confused. Need to dig into it more.

Something with the linking in the editor doesn't seem to work since the sort method is missing here. It should be okay to only change the code changed here so I don't think we need extra docs.

I can't speak to why the relevant code is missing from that localhost screenshot (that does seem odd), but adding the method to codegen.js seems to have successfully added sort to the editor preview. (see the test link on #3973 ).

From revisiting the package descriptions, it looks like vega-interpreter is only used for the CSP compliant mode.

Since the editor doesn't run in CSP mode, it didn't call vega-interpreter and wouldn't know about this function.

vega-expression appears to depend on the contents of vega-functions, so I suppose it makes sense to add an entry there. If vega-expression shouldn't depend on functionContext, that's a separate issue to sort out.

(Apologies @joelostblom , as I steered you incorrectly when I suggested vega-interpreter as the package to change!)

I'm not sure about how extensively we need to test here (I still think having a realistic Vega spec that shows the application of this sort function is desirable), but I'll add something more isolated in the downstream PR for consideration in vega-functions for the unit test.

Moved to #3973