fix(ui): don't use Buffer for FNV hash
#11766
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Buffers require a polyfill in client-side JS - aac4f89 upgraded to Webpack v5, which [no longer auto adds Node polyfills](https://github.com/webpack/changelog-v5#automatic-nodejs-polyfills-removed) - this seems to be the only one we use (as far as I can tell) -- and this code quite literally copies server-side Go code, so makes sense that it may use server-side libraries - add comments in the code about where this is copied from so that it is easier to track (I had to go through the `blame` + commit history) - use `charCodeAt` to extract an int from the string instead of converting the string to a Buffer - was inpsired by looking at some JS FNV implementations and stumbled upon some lines like this: https://github.com/schwarzkopfb/fnv1a/blob/2c5ee2889e42a6bd38c64a29b227eaf9e048e40d/index.js#L22 - unit tests still pass, so think the implementation is equivalent and we're good! - also re-enable the `tslint` rule at the end of the code Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
3 tasks
agilgur5
commented
Sep 6, 2023
Signed-off-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
toyamagu-2021
approved these changes
Sep 6, 2023
terrytangyuan
approved these changes
Sep 6, 2023
the logic, as far as I can tell, is that when the name is too long it needs to be shortened, so a hash is used. The back-end PR is #6712 which also has a few back-links to other issues and PRs. Could also ask JP if he remembers (he is currently active on both Slack and GitHub -- can tag him here if you're curious) I've definitely had some long names before and it's common to do some truncation or hashing when names are too long for k8s's limit. So it makes sense to me. |
|
Thanks for clarifying! |
8 tasks
terrytangyuan
pushed a commit
that referenced
this pull request
Oct 19, 2023
dpadhiar
pushed a commit
to dpadhiar/argo-workflows
that referenced
this pull request
May 9, 2024
Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #11764
Motivation
Bufferrequires a polyfill in client-side JSblame+ commit history)Modifications
use
charCodeAtto extract an int from a character of a string instead of converting the string to a Bufferalso re-enable the
tslintrule at the end of the codeVerification
Misc Notes
I decided not to replace the FNV hash implementation with a library as basically all of them are unmaintained and this change only ended up being a few LoC. Some of the libraries are also quite large (implementing many different algorithms and are not tree-shakeable), or have build issues, or use BigInt (which is not supported in older browsers... although those are all EoL and I wouldn't personally support such old browsers, but still something to consider).