Skip to content

feat: Add base64 string encoder/decoder to vega-expression and vega-interpreter#4009

Merged
hydrosquall merged 5 commits intomainfrom
cameron.yick/feature/add-base64-encode-expression
Feb 15, 2025
Merged

feat: Add base64 string encoder/decoder to vega-expression and vega-interpreter#4009
hydrosquall merged 5 commits intomainfrom
cameron.yick/feature/add-base64-encode-expression

Conversation

@hydrosquall
Copy link
Member

@hydrosquall hydrosquall commented Feb 4, 2025

Motivation

Testing

Notes

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 4, 2025

Deploying vega with  Cloudflare Pages  Cloudflare Pages

Latest commit: d51ef3a
Status: ✅  Deploy successful!
Preview URL: https://792cf377.vega-628.pages.dev
Branch Preview URL: https://cameron-yick-feature-add-bas.vega-628.pages.dev

View logs

@hydrosquall hydrosquall force-pushed the cameron.yick/feature/add-base64-encode-expression branch 3 times, most recently from 0437640 to fd926b9 Compare February 4, 2025 03:13
@hydrosquall hydrosquall force-pushed the cameron.yick/feature/add-base64-encode-expression branch from 57ad81c to 4831149 Compare February 12, 2025 03:31
@hydrosquall hydrosquall changed the title feat: Add base64 string encoder to expression language feat: Add base64 string encoder/decoder to vega-expression and vega-interpreter Feb 12, 2025
@hydrosquall hydrosquall force-pushed the cameron.yick/feature/add-base64-encode-expression branch from 4831149 to 05bf76e Compare February 12, 2025 03:38
@hydrosquall hydrosquall force-pushed the cameron.yick/feature/add-base64-encode-expression branch from 05bf76e to f56cf1f Compare February 13, 2025 04:05
@hydrosquall hydrosquall self-assigned this Feb 14, 2025
@hydrosquall hydrosquall marked this pull request as ready for review February 14, 2025 03:07
@hydrosquall hydrosquall requested a review from a team as a code owner February 14, 2025 03:07
trim: x => String(x).trim(),
// Base64 encode/decode
// Convert binary string to base64-encoded ascii
btoa: x => btoa(x),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this just be

Suggested change
btoa: x => btoa(x),
btoa: btoa,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could. A small difference is wrapping btoa in a function ensures that our public API ensures that only 1 argument is ever passed to btoa.

This is similar to how writing ['1','2'].map(parseInt) and ['1','2'].map((num) => parseInt(num)) are not equivalent

['5','2'].map((num) => parseInt(num))
// returns [5, 2]
['5','2'].map(parseInt)
// returns [5, NaN]

The NaN appears as a result of calling parseInt(2, 1) since the index of the array is passed as the second arg of the mapping function, and changes the radix to something other than base 10. We want to avoid that.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test

@hydrosquall hydrosquall force-pushed the cameron.yick/feature/add-base64-encode-expression branch from 93f4e75 to cb92386 Compare February 14, 2025 20:15
@hydrosquall hydrosquall force-pushed the cameron.yick/feature/add-base64-encode-expression branch from cb92386 to d51ef3a Compare February 14, 2025 20:50
@hydrosquall
Copy link
Member Author

hydrosquall commented Feb 14, 2025

@domoritz I have added both a unit test for vega-functions and an interpreter-mode test for vega-interpreter.

It felt a bit heavy to add 2 big JSONs just to test these two functions, but that appears to be the pattern for vega-interpreter (rather than testing just the function in isolation). Let me know if you have feedback about naming/content of the scenegraph test.

@hydrosquall hydrosquall requested a review from domoritz February 14, 2025 22:05
@domoritz
Copy link
Member

I had a similar reaction when I saw the tests but it's fine.

@hydrosquall hydrosquall merged commit a3af49e into main Feb 15, 2025
6 checks passed
@hydrosquall hydrosquall deleted the cameron.yick/feature/add-base64-encode-expression branch February 15, 2025 04:21
@lsh lsh mentioned this pull request Feb 24, 2025
lsh added a commit that referenced this pull request Feb 25, 2025
Changes since v5.31.0

**vega-expression**
* Add base64 string encoder/decoder to `vega-expression` and
`vega-interpreter` (via #4009). (Thanks @hydrosquall!)

**vega-typings**
* Add Typescript Types for `vega-loader` (via #4000). (Thanks
@hydrosquall!)

**docs**
* Correct data year citation in dorling-cartogram example (via #4006).
(Thanks @dsmedia!)
* Update typo in vega.timeFloor description (via #4010). (Thanks
@hydrosquall!)
* Add Security Advisory Policy for Vega (via #4008). (Thanks
@hydrosquall!)
* Replace redirect url in `expressions.md` (via #3996). (Thanks
@dangotbanned!)
* correct queries to query in `crossfilter.md` (via #4005). (Thanks
@danmarshall!)

---------

Signed-off-by: Lukas Hermann <1734032+lsh@users.noreply.github.com>
hydrosquall added a commit that referenced this pull request Mar 3, 2025
…ecode (atob, btoa) (#4023)

## Motivation

- When #4009 was released, we didn't
know the version number it would be used. Now we do!

## Changes

- Replace 5.3x placeholder with 5.32.0, which is the correct version
where https://github.com/vega/vega/releases/tag/v5.32.0 was made
available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants