Replace sha1 npm package with inline implementation#1465
Conversation
…polyfill from browser bundles The sha1 npm package references Node.js Buffer internally, causing bundlers (webpack/Next.js) to inject a ~28 KB Buffer polyfill into every client page. Since the tracker only hashes short ASCII domain+path strings, a lightweight inline SHA-1 (FIPS 180-4) replaces the dependency entirely. Fixes snowplow#1464
|
Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign. Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. |
Snowplow CLA bot (snowplowcla)
added
the
cla:no
|
Confirmed! Jan Nicklas (@jantimon) has signed the Contributor License Agreement. Thanks so much. |
Snowplow CLA bot (snowplowcla)
added
cla:yes
There was a problem hiding this comment.
Pull request overview
This PR removes the sha1 npm dependency from @snowplow/browser-tracker-core and replaces it with an inline SHA-1 implementation to avoid bundlers pulling in a Buffer polyfill and to remove the last CommonJS dependency from the ESM build.
Changes:
- Replaced
sha1dependency usage with an internalsha1(message: string)helper implementation. - Removed
sha1/@types/sha1from dependencies and updated Rush/PNPM metadata accordingly. - Added Jest coverage for SHA-1 RFC vectors and domain-hash equivalence checks.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libraries/browser-tracker-core/src/helpers/sha1.ts | New inline SHA-1 implementation used in place of the sha1 package. |
| libraries/browser-tracker-core/src/tracker/index.ts | Switched domain-hash computation to use the new internal SHA-1 helper. |
| libraries/browser-tracker-core/test/helpers/sha1.test.ts | Added SHA-1 unit tests (RFC vectors + domain-hash checks). |
| libraries/browser-tracker-core/package.json | Removed sha1 and @types/sha1 dependencies. |
| common/config/rush/pnpm-lock.yaml | Updated lockfile to remove sha1 and its transitive deps. |
| common/config/rush/repo-state.json | Updated Rush shrinkwrap hash due to lockfile changes. |
| common/config/rush/browser-approved-packages.json | Removed approvals for sha1 and @types/sha1. |
| common/autoinstallers/rush-prettier/pnpm-lock.yaml | Regenerated prettier autoinstaller lockfile (format + resolved versions). |
Files not reviewed (2)
- common/autoinstallers/rush-prettier/pnpm-lock.yaml: Language not supported
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
according to .browserslistrc IE11 is supported so we don't use padStart Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Matus Tomlein (@matus-tomlein) all Copilot remarks have been resolved - could you please take a look? |
Matus Tomlein (matus-tomlein)
left a comment
Matus Tomlein (matus-tomlein)
left a comment
There was a problem hiding this comment.
Thank you for the contribution Jan Nicklas (@jantimon), that's a great improvement in the package size! Have run some more tests and everything seems to be correct, so we will try to get this out in a release soon.
|
Jan Nicklas (@jantimon) could you please point this PR into the |
…-replace-sha1-buffer-polyfill # Conflicts: # common/config/rush/pnpm-lock.yaml # common/config/rush/repo-state.json
|
Matus Tomlein (@matus-tomlein) done - had to fix the merge conflicts but it looks good now |
Matus Tomlein (matus-tomlein)
merged commit ff51210
into
snowplow:release/4.7.0
Fixes #1464 The `sha1` npm package references `Buffer.isBuffer()` internally. Bundlers like webpack/Next.js see this and inject a **~28 KB Buffer polyfill** into every client page — even though the `Buffer` code path is never reached (we only hash short strings) On top of that, `sha1` only ships CommonJS, forcing CJS interop on every bundler that consumes the ESM dist - Remove `sha1` and `@types/sha1` dependencies - Add a ~90-line inline SHA-1 (FIPS 180-4) that only handles UTF-8 strings — which is all the tracker needs - Tests cover RFC 3174 vectors plus the actual domain-hash values to ensure identical output - **-28 KB raw / ~7 KB gzipped** from every page for Next.js / webpack 5 users - Eliminates the last CJS dependency in `browser-tracker-core` - No API or behavioral changes
Fixes #1464
The
sha1npm package referencesBuffer.isBuffer()internally. Bundlers like webpack/Next.js see this and inject a ~28 KB Buffer polyfill into every client page — even though theBuffercode path is never reached (we only hash short strings)On top of that,
sha1only ships CommonJS, forcing CJS interop on every bundler that consumes the ESM distRemove
sha1and@types/sha1dependenciesAdd a ~90-line inline SHA-1 (FIPS 180-4) that only handles UTF-8 strings — which is all the tracker needs
Tests cover RFC 3174 vectors plus the actual domain-hash values to ensure identical output
-28 KB raw / ~7 KB gzipped from every page for Next.js / webpack 5 users
Eliminates the last CJS dependency in
browser-tracker-coreNo API or behavioral changes