feat(sep): add dedicated /sep entrypoint for SEP-1/2/10 helpers - #3
feat(sep): add dedicated /sep entrypoint for SEP-1/2/10 helpers#3Dione-b wants to merge 1 commit into
Conversation
pedro-pelicioni
left a comment
There was a problem hiding this comment.
Packaging itself is fine — I built the branch and both artifacts resolve at runtime (lib/esm/sep/index.js, lib/cjs/sep/index.js, lib/esm/sep/index.d.ts), the exports shape matches the existing subpaths, and node tests pass 7/7. Two blockers and some description issues.
1. Breaks CI. test/unit/sep_entrypoint.test.ts fails in both browsers:
FAIL |chromium| test/unit/sep_entrypoint.test.ts
Caused by: Error: Module "fs" has been externalized for browser compatibility.
Cannot access "fs.readFileSync" in client code.
FAIL |firefox| same
config/vitest.config.browser.ts has include: ["test/unit/**/*.test.ts"], and .github/workflows/tests.yml:69,72 runs test:browser / test:browser:axios. The PR only claims test:node, so this went unnoticed. The precedent is already in the repo: test/unit/guide-snippets.test.ts is explicitly excluded for exactly this reason. Either add this file to that exclude list, or drop the fs assertions (see 4).
2. Missing CHANGELOG entry. /base got one under ### Added (stellar#1550) — same convention applies here.
3. The stated motivation doesn't hold for the main case. "They ship in every bundle anyway" isn't true for ESM bundler consumers — I measured import { Keypair, StrKey } from the root at 44 KB gzip, with 0 references to webauth in the unminified bundle. It already tree-shakes (sideEffects is a narrow allowlist). The real beneficiaries are CJS/require, import * as sdk, and bundlers that handle export * as poorly. That's a legitimate argument and it carries the PR — but it's a different one, worth rewording.
4. The numbers don't reproduce. esbuild, minified + gzip: /sep = 79 KB, root named import = 44 KB, full root namespace = 118 KB. /sep costs more than a named root import, which makes sense since WebAuth pulls the whole transaction-building stack. The honest framing is 79 vs 118 for namespace-import consumers — a real ~39 KB saving, just not 43 vs 247. Worth publishing the measurement script.
5. Two of the three test groups are change-detectors. The rollup one (expect(config).toMatch(/"sep\/index":\s*"src\/sep\/index\.ts"/)) breaks if prettier changes quote style, with nothing actually broken. The valuable assertion is expect(sep[name]).toBe(sdk[name]) — it catches a real regression (forked impl / different http-client) and is browser-safe. Keeping only that one also resolves blocker 1. If the packaging invariant deserves a guard, assert on the build output, not on config text.
6. Base branch is dione/develop — make sure the upstream PR targets master.
StellarToml, Federation and WebAuth are the SDK's network-fetching helpers and the ones most consumers never touch, but they're only reachable from the package root. Adds a `/sep` subpath (and `/axios/sep`), following the pattern set by `/base`, `/rpc` and `/contract`, so callers can opt in explicitly. Purely additive -- the root export is unchanged. ESM consumers on a modern bundler already get this for free: `sideEffects` is a narrow allowlist, so a named import from the root tree-shakes the SEP modules out. The subpath is for the cases where that doesn't happen -- `require()`, `import * as sdk`, and bundlers that handle `export * as` poorly -- where today the whole root graph is pulled in regardless. Measured with esbuild (bundle, minify, browser, gzip) via scripts/measure-bundle.mjs: root, namespace import 120.0 KB root, named import (Keypair, StrKey) 45.4 KB /sep, namespace import 80.5 KB So `/sep` saves a namespace-import consumer ~39 KB. It is not smaller than a tree-shaken named import from the root -- WebAuth pulls in the whole transaction-building stack -- and it isn't meant to be. Co-authored-by: Nearx-Labs <nearxlabs@nearx.com.br>
d70063c to
e32ffe9
Compare
|
What problem does your feature solve?
Your numbers reproduce; mine were wrong. esbuild (bundle, minify, browser, gzip), against the built
So ~39 KB for namespace-import consumers, and What would you like to see? A Fixed in the amended commit:
|
|
Both blockers are cleared — verified on
One thing left: the PR description wasn't updated and now contradicts the script the PR itself adds.
Description sync only; the code looks good to me. |
|
@pedro-pelicioni description updated — no code changes.
|
What problem does your feature solve?
SEP helpers (stellar.toml/SEP-1, federation/SEP-2, web auth/SEP-10) are only exported from the package root, mixed in with core primitives. Consumers who want just these helpers have no way to ask for them alone:
require()consumers,import * as sdknamespace imports, and bundlers that don't tree-shakeexport * aswell all end up pulling in Horizon and RPC. And these are the modules that do network fetches (see the SSRF fix in stellar#1390), so a SEP-specific concern touches every SDK user.Modern bundlers doing named ESM imports already tree-shake the root well — this is not a fix for that case, it's a fix for the cases that can't.
What would you like to see?
A
@stellar/stellar-sdk/sepsubpath, following the/base,/rpc,/contractpattern. Purely additive, 6 files:src/sep/index.ts(new) — barrel re-exportingFederation,StellarToml,WebAuthpackage.json—./sepand./axios/sepinexportsrollup.config.mjs— one line inlibEntriestest/unit/sep_entrypoint.test.ts(new) — keeps the three in lockstepscripts/measure-bundle.mjs(new) — reproducible bundle measurementCHANGELOG.mdsrc/index.tsuntouched — nothing moves, nothing breaks.Measurements
Run
node scripts/measure-bundle.mjsto reproduce:The relevant comparison is namespace-to-namespace: 79.8 KB gzip via
/sepvs 119.2 KB via the root. The named-import row is there to show the honest baseline — if your bundler can do that, you already get the smallest result and don't need this entrypoint.Issue reference
Closes stellar#1474
pnpm run test:node/test:node:axios(110 files, 2235 tests),pnpm run test:browser(chromium + firefox), andpnpm run _buildpass; all 4 build artifacts verified at runtime.