Conversation
|
@Blipz is attempting to deploy a commit to the azgaar's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR packages Armoria as a consumable library by adding a dedicated packaging build, introducing a public library entrypoint, and adapting runtime code to work outside direct SvelteKit $app/* imports.
Changes:
- Add a packaging build flow (
vite.pkg.config.ts+npm run pkg) and libraryexportsinpackage.json. - Replace
$app/environmentusage withesm-envBROWSERchecks in shared runtime modules. - Extend generation/rendering APIs for external/library usage (customizable generation hooks; renderer supports a browser path).
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
vite.pkg.config.ts |
Adds Vite “lib mode” build config for packaging output. |
src/lib/scripts/getters.js |
Switches browser detection to esm-env for non-SvelteKit consumers. |
src/lib/scripts/generator.js |
Adds custom hooks to influence generation and finalize output. |
src/lib/index.ts |
Introduces the library entrypoint exports. |
src/lib/data/stores.ts |
Switches browser detection to esm-env for non-SvelteKit consumers. |
src/lib/components/object/Shield.svelte |
Avoids DOM-side pattern/charge injection when rendered as “External”. |
src/lib/components/object/COA.svelte |
Adjusts SVG id handling for “External” rendering. |
src/lib/api/renderer.js |
Adds browser rendering path and makes logging optional; changes charge-loading logic accordingly. |
package.json |
Adds pkg script, adds @sveltejs/package, sets peers, and defines package exports. |
package-lock.json |
Locks dependency updates for the new packaging dependency. |
.gitignore |
Ignores /dist output. |
Comments suppressed due to low confidence (1)
src/lib/api/renderer.js:31
render()now has a browser code path, but it still callsgetFonts()unconditionally.getFonts()/loadFontsAsDataURI()currently usesBuffer.from(...), which will throw in the browser unless a Buffer polyfill is present. Consider skipping font embedding in the browser path, or implementing a browser-safe base64 conversion (or making font embedding an opt-in server-only feature).
const loadedCharges = await getCharges(coa, shieldPath);
const loadedPatterns = getPatterns(coa);
const loadedFonts = await getFonts(coa);
const shieldClip = `<clipPath id="shield_${coa.seed}"><path d="${shieldPath}"/></clipPath>`;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "format": "prettier --write .", | ||
| "postinstall": "patch-package" | ||
| "postinstall": "patch-package", | ||
| "pkg": "svelte-package && vite -c vite.pkg.config.ts build && cp package.json dist/ && cp -r static/charges dist/" |
There was a problem hiding this comment.
The pkg script assumes the intermediate and final artifacts live in dist/ (and copies package.json + static/charges there), but svelte-package will not output to dist/ unless explicitly configured. As written, vite -c vite.pkg.config.ts build may fail because its entry is expected at dist/index.js. Align the output directories (configure svelte-package output dir, and/or adjust the Vite entry/outDir) so the pipeline is reproducible.
| "pkg": "svelte-package && vite -c vite.pkg.config.ts build && cp package.json dist/ && cp -r static/charges dist/" | |
| "pkg": "svelte-package && rm -rf dist && cp -r package dist && vite -c vite.pkg.config.ts build && cp package.json dist/ && cp -r static/charges dist/" |
| const customTincture = custom?.tincture(); | ||
| if (customTincture) { | ||
| if (customTincture.includes("-")) config.usedPattern = customTincture; | ||
| else config.usedTinctures.push(customTincture); | ||
| } | ||
| const coa = {seed, t1: customTincture || getTincture(config, "field")}; |
There was a problem hiding this comment.
custom?.tincture() (and the similar calls to custom?.ordinary(), custom?.division(), custom?.charge(), custom?.finalize()) will throw if custom is provided but the corresponding property is missing/non-function. Use optional chaining on the method itself (e.g. custom.tincture?.()), or validate custom's shape up front before calling into it.
| new COA({target: root, props: {coa, height: size, width: size, i: "External"}}); | ||
| root.querySelector("defs").innerHTML = | ||
| `${shieldClip}${divisionClip}${loadedCharges}${loadedPatterns}${loadedFonts}${backlight}${style}`; | ||
| return root.innerHTML; |
There was a problem hiding this comment.
In the browser branch, a COA component instance is created but never destroyed. If render() is called repeatedly, this can leak store subscriptions and reactive effects. Capture the component instance and call $destroy() after extracting the SVG HTML.
| new COA({target: root, props: {coa, height: size, width: size, i: "External"}}); | |
| root.querySelector("defs").innerHTML = | |
| `${shieldClip}${divisionClip}${loadedCharges}${loadedPatterns}${loadedFonts}${backlight}${style}`; | |
| return root.innerHTML; | |
| const coaComponent = new COA({target: root, props: {coa, height: size, width: size, i: "External"}}); | |
| root.querySelector("defs").innerHTML = | |
| `${shieldClip}${divisionClip}${loadedCharges}${loadedPatterns}${loadedFonts}${backlight}${style}`; | |
| const html = root.innerHTML; | |
| coaComponent.$destroy(); | |
| return html; |
| if (browser) { | ||
| const fetched = fetch(`./charges/${charge}.svg`) | ||
| .then((res) => { | ||
| if (res.ok) return res.text(); | ||
| else throw new Error("Cannot fetch charge"); | ||
| }) |
There was a problem hiding this comment.
fetch(./charges/${charge}.svg) is relative to the current URL path; in apps with nested routes or a non-root base it can resolve incorrectly. Elsewhere (e.g. $lib/scripts/getters) charges are fetched via charges/.... Consider using an absolute path (/charges/...), SvelteKit paths.base, or accept a base URL parameter so consumers can control where charge assets are served from.
| plugins: [svelte()], | ||
| build: { | ||
| lib: { | ||
| entry: 'dist/index.js', |
There was a problem hiding this comment.
build.lib.entry points at dist/index.js, but the pkg script runs svelte-package with no configuration/flags to output to dist (the default output dir is typically package/). Also, Vite’s default outDir is dist and it empties the directory before building, which can delete the entry file. Consider (a) setting svelte-package --output dist (or configuring the package output dir), and (b) using a different outDir for the Vite bundle or emptyOutDir: false to avoid deleting the entry.
| entry: 'dist/index.js', | |
| entry: 'package/index.js', |
|
@Blipz , should we merge this one first? |
|
No, we can't because it depends on the API being available in Armoria. |
No description provided.