Open
Conversation
Contributor
|
@micha-lmxt please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Author
|
@microsoft-github-policy-service agree |
6 tasks
vitbokisch
added a commit
to pyreon/pyreon
that referenced
this pull request
Apr 14, 2026
Resolves the deferred Phase 3 PR #231 charts blocker. ═══ ROOT CAUSE ═══ ECharts's lib files do `import { __extends } from "tslib"`. tslib's package.json `exports` maps the `import` condition to `./modules/index.js`, which is a tiny wrapper that does: import tslib from '../tslib.js' const { __extends, __assign, __rest, ... } = tslib export { __extends, __assign, __rest, ... } `tslib.js` is a UMD/CJS file that exposes helpers as TOP-LEVEL `var` declarations on the factory exports object — NOT as properties of `module.exports.default`. When esbuild's pre-bundler (used by Vite under @vitest/browser) wraps `tslib.js` via `__toESM(require_tslib())`, the destructure throws: TypeError: Cannot destructure property '__extends' of '__toESM(...).default' as it is undefined. Known upstream issue: microsoft/tslib#189. ═══ FIX ═══ Alias `tslib` → `tslib.es6.js` in `vitest.browser.config.ts`. `tslib.es6.js` is a flat ESM module with `export function __extends(...)` — sidesteps the broken `modules/index.js` indirection entirely. resolve: { alias: { tslib: '<echarts-dir>/../tslib/tslib.es6.js' } } Resolved via `createRequire` → `echarts/package.json` since tslib isn't a direct dep of @pyreon/charts. The `module` condition in tslib's exports map points at `tslib.es6.js`, but Vite's resolver under vitest browser mode + the workspace `bun` condition lands on `default` (CJS) instead. Explicit alias is the most robust fix. Phase 3 PR #231 tried `optimizeDeps.include` for echarts subpaths + tslib + zrender, `resolve.mainFields` reorder, and bare `resolve.alias` to tslib's ESM entry. The first two didn't help (the failure happens deep in the optimized bundle, not at the user-facing import boundary). The alias attempt failed because the path resolved via `tslib/package.json` doesn't work — tslib isn't a direct dep — but resolving via echarts (which IS) does. ═══ TESTS ═══ charts.browser.test.tsx: 3 → 6 tests, +3 canvas tests: - lazy-loads ECharts and renders a real canvas with non-zero dimensions - reactive options getter — signal change re-applies without remounting canvas - useChart.instance() resolves to a real ECharts instance after mount; assertion includes the documented API surface (setOption, resize, dispose) ═══ BISECT-VERIFIED ═══ Removed the alias in vitest.browser.config.ts → exactly the 3 new canvas tests failed (the 3 bridge tests still pass because they don't require ECharts to load). Restored, all 6 pass. ═══ DOCS ═══ CLAUDE.md @pyreon/charts section now documents the tslib alias requirement + upstream issue link. Removes the "ECharts loading is blocked" note from PR #231. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
vitbokisch
added a commit
to pyreon/pyreon
that referenced
this pull request
Apr 14, 2026
Addresses the 8/10 self-critique: ═══ #1 SHARED HELPER (formerly in-line in charts config) ═══ `vitest.browser.ts` now exports `tslibBrowserAlias(import.meta.url)` — a robust resolver that walks both bun-nested (.bun/echarts@x/node_modules/tslib/) and hoisted (node_modules/tslib/) install layouts. Returns `{}` if tslib isn't found, so consumers can spread the alias unconditionally. `@pyreon/charts/vitest.browser.config.ts` now uses the helper: alias: { ...tslibBrowserAlias(import.meta.url) } Future packages whose transitive deps pull in tslib (rxjs, MobX, older Material UI versions, etc.) can adopt the same one-liner. ═══ #2 ROBUST RESOLUTION ═══ Old (PR #245 round 1): `createRequire(echarts/package.json)` then `../tslib/tslib.es6.js` — assumed bun's specific layout. pnpm/yarn/ npm with hoisted tslib would break. New helper walks `node_modules/tslib/tslib.es6.js` from the calling directory upward (10 levels max) AND falls back to the `createRequire(echarts)` lookup if nothing direct works. ═══ #3 AUDIT: NO OTHER PACKAGES NEED THIS ═══ Checked dependencies + peerDependencies of @pyreon/flow, @pyreon/code, @pyreon/document-primitives, @pyreon/router, @pyreon/head, @pyreon/runtime-dom — none transitively import tslib. Only echarts (via @pyreon/charts) currently triggers this issue. Future packages that hit it can apply the helper. ═══ #4 DOC PROPAGATION (was 1 surface, now 3) ═══ Added the tslib alias note + microsoft/tslib#189 link to: - llms.txt (one-liner with link) - llms-full.txt (table cell with consumer fix snippet) - charts/README.md (full "Bundler note" section with vite.config snippet for downstream apps) CLAUDE.md was already updated in PR #245 round 1. ═══ #5 BISECT GRANULARITY EXPLAINED ═══ Per-test bisect isn't load-bearing for the alias contract — all 3 canvas tests share a precondition (ECharts must load) but each asserts a DISTINCT observation: 1. dimensions test catches "canvas is 0×0" 2. reactive-options test catches "canvas remounts on signal change" 3. instance API test catches "useChart didn't expose instance signal" Comment in `charts.browser.test.tsx` documents this explicitly so the next person doesn't redo the analysis. The bundled bisect from PR #245 round 1 (alias gone → all 3 fail) is the right shape for the alias contract specifically. ═══ #6 TEST-ONLY SCOPE CLARIFIED ═══ `vitest.browser.config.ts` comment now explicitly notes this is a test-environment fix and Pyreon's published `lib/*.js` ships without the alias. Consumer apps using Vite hit the same bug; the fix is documented in `charts/README.md` for them to apply in their own `vite.config.ts`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vitbokisch
added a commit
to pyreon/pyreon
that referenced
this pull request
Apr 14, 2026
## Summary Resolves the deferred Phase 3 PR #231 charts blocker — ECharts canvas tests now run reliably under @vitest/browser. ## Root cause ECharts's lib files do `import { __extends } from "tslib"`. tslib's `package.json` `exports` maps the `import` condition to `./modules/index.js`, which does: ```js import tslib from '../tslib.js' const { __extends, __assign, __rest, ... } = tslib export { __extends, __assign, __rest, ... } ``` `tslib.js` is a UMD/CJS file that exposes helpers as TOP-LEVEL `var` declarations on the factory exports object — NOT as properties of `module.exports.default`. When esbuild's pre-bundler (used by Vite under @vitest/browser) wraps `tslib.js` via `__toESM(require_tslib())`, the destructure throws: ``` TypeError: Cannot destructure property '__extends' of '__toESM(...).default' as it is undefined. ``` Known upstream issue: [microsoft/tslib#189](microsoft/tslib#189). ## Fix Alias `tslib` → `tslib.es6.js` in `vitest.browser.config.ts`. `tslib.es6.js` is a flat ESM module with `export function __extends(...)` — sidesteps the broken `modules/index.js` indirection entirely. Resolved via `createRequire` against `echarts/package.json` because tslib isn't a direct dep of `@pyreon/charts`. PR #231 tried `optimizeDeps.include` (no effect — failure is deep in optimized bundle), `resolve.mainFields` reorder (no effect — same reason), and bare `resolve.alias` to tslib's ESM entry (failed because resolving via `tslib/package.json` doesn't work). Resolving via echarts works. ## Tests `charts.browser.test.tsx`: 3 → 6 tests. New canvas-rendering tests: - lazy-loads ECharts and renders a real canvas with non-zero dimensions - reactive options getter — signal change re-applies without remounting canvas - `useChart.instance()` resolves to a real ECharts instance + asserts API surface (`setOption`, `resize`, `dispose`) All 6 pass. ## Bisect-verified Removed the alias in `vitest.browser.config.ts` → exactly the 3 new canvas tests failed (the 3 bridge tests still pass because they don't require ECharts to load). Restored, all 6 pass. ## Test plan - [ ] CI `Test (browser)` — `charts.browser.test.tsx` 6/6 pass - [ ] `bun run typecheck` — 0 errors - [ ] `bun run lint` — 0 new errors - [ ] `bun run lint:browser-smoke` — clean ## Next That clears the C queue. Phase 5 (B) — ui-system rollout, 14 packages currently in `PHASE_5_PENDING_PACKAGES`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm trying to use tslib with vite 4.0.1. Dev mode seems to demand 'type':'module' in package.json. It then goes into modules subfolder and complains that '../tslib.js' has no default export. Can't argue with that, so I added the '* as' to the import.