Skip to content

Conversation

@danpoletaev
Copy link
Contributor

This PR adds new function to the utility package.

Function createHmacSignature() will be used in Apify SDK as well as in Apify Core repositories

@danpoletaev danpoletaev added adhoc Ad-hoc unplanned task added during the sprint. t-core-services Issues with this label are in the ownership of the core services team. labels Feb 11, 2025
@github-actions github-actions bot added this to the 109th sprint - Platform team milestone Feb 11, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Feb 11, 2025
@B4nan
Copy link
Member

B4nan commented Feb 11, 2025

I am not very happy to see any additional dependencies in the utilities package. We use that in many places (crawlee core and Apify CLI to begin with), and they will now all get a new dependency, even though it will be used in only the SDK and apify-core. The function seems too trivial to justify this.

I know it's really a small dependency, but at the same time the new helper is just a 5 lines of code.

@fnesveda
Copy link
Member

@B4nan is it really such an issue? I feel like you're overoptimizing, the additional complexity from sharing the helper in multiple places is not worth 0.02% of savings in Crawlee.

> npx howfat crawlee
[email protected] (575 deps, 95.66mb, 19921 files, ©Apache-2.0)

> npx howfat @apify/utilities
@apify/[email protected] (3 deps, 695.66kb, 33 files, ©Apache-2.0)

> npx howfat base62
[email protected] (25.88kb, 18 files, ©MIT)

@B4nan
Copy link
Member

B4nan commented Feb 11, 2025

I am just pointing out a bad practice, especially when it comes to public packages. You picked the fattest library we have for your comparison.

Btw you say I am overoptimizing, I say you are over-engineering. Pick your poison :]

Anyway, I am fine with this, I just don't like it.

@danpoletaev
Copy link
Contributor Author

The encode function is pretty simple. I copied it from https://github.com/base62/base62.js/blob/master/lib/ascii.js to avoid new dependency.

Hope everyone is ok with that

Copy link
Member

@fnesveda fnesveda left a comment

Choose a reason for hiding this comment

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

I'm afraid this won't work correctly. if the signature is 30 hex characters, and you parse it to an integer, it's max possible value is 2^120, which is far larger than the max safe integer in JavaScript (Number.MAX_SAFE_INTEGER , equal to 2^53 - 1).

You'll have to use BigInts for this.

@danpoletaev danpoletaev requested a review from fnesveda February 12, 2025 08:42
Copy link
Member

@fnesveda fnesveda left a comment

Choose a reason for hiding this comment

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

Looks good, one last tiny thing

@danpoletaev danpoletaev merged commit fe937f2 into master Feb 12, 2025
9 checks passed
@danpoletaev danpoletaev deleted the feat/create-hmac-signature branch February 12, 2025 09:27
@tobice tobice added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-core-services Issues with this label are in the ownership of the core services team. tested Temporary label used only programatically for some analytics. validated Issues that are resolved and their solutions fulfill the acceptance criteria.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants