Conversation
70fa996 to
141fc3a
Compare
kmcginnes
left a comment
There was a problem hiding this comment.
This does everything issue #2180 asks for, including the ES2023 amendment. pnpm check:types and pnpm build pass, dist has no Iterator polyfill, core-js is gone from the package, lockfile, workspace, and index.tsx, and no Iterator helpers, ES2025 Set methods, or .isWellFormed() calls remain. The set helpers match native semantics and isWellFormedString handles a trailing lone high surrogate correctly.
Needs fixing
connector/utils/warnMissingIds.test.ts:3-8:vi.mock("@/utils")stubs an internal module, whichdocs/agents/testing.mdrules out. Usevi.spyOn(logger, "warn")instead.utils/setHelpers.test.ts: usetoStrictEqualinstead oftoEqual, pertesting.md.connector/utils/warnMissingIds.ts:10,16:context: Record<string, unknown>spread into the log payload goes against the AGENTS.md rules on named domain types and spreads. The genericTalso accepts raw strings where it should beVertexId | EdgeId.utils/setHelpers.ts:9,13,20: all three helpers spread the set into an array. These run on graph-sized sets inuseRemoveFromGraph, so plainfor…ofloops would avoid the copies.
Worth considering
- Some rewrites copy more than they need to, where the issue's
Array.from(iter, fn)form or a loop would do:neighbors.ts~111 doesArray.from().filter().map(), making three arrays.neighbors.ts~146 copies only toreduce.sparql/fetchNeighbors/index.ts~52 andFakeExplorer.ts~139 copy then filter.
sparql/fetchSchema/index.ts~138,144:Array.from(candidates, map).find()maps every candidate instead of stopping at the first match. The result is the same.displayEdge.ts~50 anddisplayVertex.ts~60: thee is Edge/n is Vertexpredicates aren't needed, since TS 5.5+ infers them.- The
warnMissingIdsextraction goes beyond what the issue asked for. It's fine to keep, but it's the source of two of the fixes above.
|
I benchmarked the old and new code at 10 to 100k items. The runs were in Node 26, which uses V8, so the results should match Chrome. The "before" measurements used both the native iterator helpers and the core-js polyfill that floor browsers were actually running. One change to make:
With the mapping function, the new code is only as fast as the old native helpers. I'd switch those call sites to The rest
Every variant returned the same result as the new code at every size. |
…can be removed Replace Iterator helper chains (Map/Set .values().toArray(), .map(), .filter(), .reduce(), .forEach()) with Array.from and native array methods, which are supported in our browser floor. Replace ES2025 Set methods (.difference, .union, .isSubsetOf) with small helpers in src/utils/setHelpers.ts. Replace String.prototype.isWellFormed() with a manual surrogate check in src/utils/isWellFormedString.ts. Narrow TypeScript lib to ES2023 in graph-explorer and shared to prevent the above-floor APIs from being reintroduced. Pin the Vite build target to baseline-widely-available and remove the core-js dependency and import. Keep pnpm-workspace.yaml allowBuilds: core-js: false as a guard against accidental reintroduction. Issue: aws#2180
Extract warnMissingIds to replace the duplicated missing-ID warning blocks across the connector detail fetchers, rewrite the session and graph-update ID collection as single-pass loops, import setHelpers through the utils barrel, drop the manual useMemo in DataExplorer in favour of the compiler-memoized hoisted form, and remove the dangling core-js allowBuilds entry.
- Constrain warnMissingIds to VertexId/EdgeId and a named context type - Replace the internal-module mock with a spy on logger.warn - Implement setHelpers with for...of loops instead of array copies - Use toStrictEqual in setHelpers tests - Remove redundant type predicates in displayEdge/displayVertex selectors - Rewrite neighbors.ts ID collection as single-pass loops Generated with [Devin](https://devin.ai)
- Check well-formedness with a Unicode property regex and test it against encodeURIComponent - Map after Array.from rather than through its callback, which V8 runs slower - Restore the early exit when picking SPARQL display attributes - Replace the iterator chain main added in connectionLink - Import Vitest globals in the new tests Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
@mjuarros I rebased this onto latest Rebase fallout
Smaller
|
Description
Removes the
core-jsdependency by replacing the runtime features it was polyfilling with native equivalents that fit the project's browser floor (baseline-widely-available).Map/Set.prototype.values().toArray(),.map(),.filter(),.reduce(),.forEach(), andIterator.from(...)) to useArray.fromand native array methods..difference,.union,.isSubsetOf) with small helpers insrc/utils/setHelpers.ts.String.prototype.isWellFormed()with a manual surrogate-pair check insrc/utils/isWellFormedString.ts.libtoES2023inpackages/graph-explorer/tsconfig.jsonandpackages/shared/tsconfig.jsonso these APIs are caught at compile time.build.targetto"baseline-widely-available"invite.config.ts.Why
ES2023instead ofES2024Issue #2180 originally proposed narrowing
libtoES2024. After comparing both values against the current codebase,ES2023was the better fit for the project's browser floor.ES2024still allowsString.prototype.isWellFormed(), which is part of the ES2024 spec but is not supported by Firefox 114, the Firefox version in Vite'sbaseline-widely-availabletarget. Narrowing toES2023surfaces those call sites at compile time and forces a native replacement.ES2024would also miss the ES2025 Set-method usages (Set.prototype.difference,.union,.isSubsetOf). Those are caught by the broader refactor and replaced with native helpers.ES2023produces overES2024are fromisWellFormed(), which genuinely breaks the browser floor, so the stricter lib is the safer guard.How to read
https://github.com/aws/graph-explorer/pull/2245/files#diff-9a123d9c986e6d49b25941b73487100452414c4e1b2f4b80a8b7ef062f3c2f5c— narrowlibtoES2023https://github.com/aws/graph-explorer/pull/2245/files#diff-28fbbd10733902283f23978b34aacb8284bc7af50d729f6d0ea1526ba625d322— narrowlibtoES2023https://github.com/aws/graph-explorer/pull/2245/files#diff-4a588c5a806e7338c171574737e2e2b9525c1321a470c82ced19bbb218d0a029— pin the Vite build targethttps://github.com/aws/graph-explorer/pull/2245/files#diff-85ae7d1ae920636a625625b7bf63fe515947fb198af362302ee0644b68c7eb16— remove thecore-jsimporthttps://github.com/aws/graph-explorer/pull/2245/files#diff-d01b423527b0f3e6325bc62c4b5b434ef06f611a1568eec1fc10a1fea8eebd5a— native Set operationshttps://github.com/aws/graph-explorer/pull/2245/files#diff-1a911443adede3eb4f1befb176f8395717945020c0264741eebe9bcc418d110f— manual surrogate-pair checkhttps://github.com/aws/graph-explorer/pull/2245/files#diff-9b35d78dbdcdd2187afcd580799799d3a3d37af3af11ea05f67e7c1d34fdeaf6— adoptisWellFormedStringhttps://github.com/aws/graph-explorer/pull/2245/files#diff-5a58b234d93d11c6d703b1dbfa281f9ae66e9acad3c4a0c432dc41fe202d4fec— replaceIterator.fromhttps://github.com/aws/graph-explorer/pull/2245/files#diff-8a0f706cc93c2b7b17a17795a8a2dd21b9bb3fc4d7df5f07c97f0033014ef31f— replace Set method chainsValidation
pnpm checkspasses.pnpm testpasses (2,721 tests).pnpm buildpasses.setHelpersandisWellFormedString.Related Issues
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.