Core | Bundle: Enable tree-shaken root bundle - #740
Conversation
|
CLA Assistant Lite bot ✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
I have hereby read the F5 CLA and agree to its terms |
6fdb730 to
41fbd06
Compare
|
@onmax Thanks for the contribution! Could you please tell more about this PR, was tree-shaking not working for you with the existing configuration? Currently, Rollup creates individual modules for each TypeScript file in the code, and the actual tree-shaking is done by the user's bundler who uses Unovis. So I don't think this change will have any effect. |
|
Also your PR description mentions
but I don't see anything like this in the code. |
|
hey @rokotyan, for context I am trying to deploy this PR: nimiq/validators-api#137 This PR uses In the cloudflare ci at build time I get this error: Details
22:14:54.227 | FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory -- | -- 22:14:54.227 | ----- Native stack trace ----- 22:14:54.227 | 22:14:54.246 | 1: 0xe13fde node::OOMErrorHandler(char const*, v8::OOMDetails const&) [node] 22:14:54.246 | 2: 0x11d5070 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node] 22:14:54.247 | 3: 0x11d5347 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node] 22:14:54.248 | 4: 0x1402c05 [node] 22:14:54.249 | 5: 0x141c499 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node] 22:14:54.250 | 6: 0x13f0b48 v8::internal::HeapAllocator::AllocateRawWithLightRetrySlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node] 22:14:54.250 | 7: 0x13f1a75 v8::internal::HeapAllocator::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [node] 22:14:54.251 | 8: 0x13ca74e v8::internal::Factory::NewFillerObject(int, v8::internal::AllocationAlignment, v8::internal::AllocationType, v8::internal::AllocationOrigin) [node] 22:14:54.252 | 9: 0x182bef0 v8::internal::Runtime_AllocateInOldGeneration(int, unsigned long*, v8::internal::Isolate*) [node] 22:14:54.252 | 10: 0x7fd7c3eac476 22:14:55.673 | Aborted (core dumped) 22:14:55.740 | ELIFECYCLE Command failed with exit code 134. 22:14:55.787 | Failed: error occurred while running build command I can try to reproduce a minimal error if you like with just unovis |
git clone --depth 1 --filter=blob:none --sparse https://github.com/onmax/repros.git
cd repros && git sparse-checkout set unovis-740
cd unovis-740 && pnpm i && pnpm build:oomImporting only |
|
@onmax Thank for the context. Does the suggested change fix the problem? Also, I wonder how it changes the built files in the |
|
Haven't tested the PR's fix against the repro yet — will do that and compare the dist outputs. In the meantime I added a git clone --depth 1 --filter=blob:none --sparse https://github.com/onmax/repros.git
cd repros && git sparse-checkout set unovis-740
cd unovis-740 && pnpm i && pnpm verifyOutput on So just importing |
|
@onmax Thanks, it would be great to get to the root of this and fix it if possible. Maybe we have circular dependencies somewhere. You can also do granular imports like |
|
Tested the PR against the repro. Here are the results: dist comparison (main vs PR)
The PR removes 16 intermediate barrle files and the side-effect // index.js (main)
-import './containers.js';
-import './components.js';
-import './data-models/index.js';
-import './types.js';
-import './utils/index.js';
export { colors, colorsDark, ... } from './styles/colors.js';
export { ContainerCore } from './core/container/index.js';
// ...same named exports in bothDoes it fix the OOM?No. Both main and PR produce the same result when consumed:
The PR removes the side-effect barrel imports, but What does fix itGranular/subpath imports bypass the barrel entirely: -import { CurveType } from '@unovis/ts'
+import { CurveType } from '@unovis/ts/types/curve'
Repro: |
|
@onmax So the problem is somewhere else: elkjs, three and maplibre (not Leaflet) are already lazy loaded. Also we want to have the barrel exports, they are convenient. And they don't prevent users from using granular imports. |
|
If you could delve deeper and analyze why Vite loads the entire Unovis codebase while building, and whether this can be avoided, that would be great. |
|
Update: keeping The root cause:
What’s added in
Result from local harness run:
No API migration required; root barrel stays intact. |
3ce977a to
8f4cea5
Compare
Summary
@unovis/tsroot build output instead of hard-disabling itstyles/indexmodulesWhy
When consumers bundle from
@unovis/tsroot entry, eager module loading can pull in graph/layout dependencies (elkjs, dagre stack) even for non-graph imports.This change ensures non-graph imports (for example
CurveType,Area,Line) do not include graph layout code, whileGraphimports still include expected layout dependencies.