-
Notifications
You must be signed in to change notification settings - Fork 30
fix: was defaulting to Debug build #306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures the build script no longer defaults to a Debug preset and updates the Graphviz demo HTML to use the new package layout and display the library version.
- scripts/cpp-build.sh: Comments out the Debug build preset to avoid defaulting to Debug.
- hw-graphviz.html: Switches imports to the new local/dist and CDN modules and adds a version display element.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/cpp-build.sh | Commented out the Debug preset to default to MinSizeRel instead |
| hw-graphviz.html | Updated Graphviz import paths, merged import logic, and show version |
Comments suppressed due to low confidence (2)
hw-graphviz.html:10
- [nitpick] The
idplaceholder_versionis generic; consider renaming it to something likegraphviz_versionfor clearer intent and consistency.
<h2 id="placeholder_version"></h2>
scripts/cpp-build.sh:8
- [nitpick] It would be helpful to add a brief comment explaining why the Debug preset is commented out, so future maintainers understand the rationale.
# cmake -S . -B ./build --preset vcpkg-emscripten-Debug
| const Graphviz = await import("./packages/graphviz/dist/index.js").then(m => m.Graphviz).catch(() => undefined) ?? | ||
| await import("https://cdn.jsdelivr.net/npm/@hpcc-js/wasm-graphviz/dist/index.js").then(m => m.Graphviz); |
Copilot
AI
Jul 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ?? chain only catches failures of the first import; if the CDN import on the next line fails, it will throw an unhandled rejection. Consider wrapping the combined import logic in a try/catch or adding a .catch() to the second import.
| const Graphviz = await import("./packages/graphviz/dist/index.js").then(m => m.Graphviz).catch(() => undefined) ?? | |
| await import("https://cdn.jsdelivr.net/npm/@hpcc-js/wasm-graphviz/dist/index.js").then(m => m.Graphviz); | |
| let Graphviz; | |
| try { | |
| Graphviz = await import("./packages/graphviz/dist/index.js").then(m => m.Graphviz); | |
| } catch (localError) { | |
| console.error("Failed to load local Graphviz module:", localError); | |
| try { | |
| Graphviz = await import("https://cdn.jsdelivr.net/npm/@hpcc-js/wasm-graphviz/dist/index.js").then(m => m.Graphviz); | |
| } catch (cdnError) { | |
| console.error("Failed to load Graphviz from CDN:", cdnError); | |
| Graphviz = undefined; | |
| } | |
| } |
| const Graphviz = await import("./packages/graphviz/dist/index.js").then(m => m.Graphviz).catch(() => undefined) ?? | ||
| await import("https://cdn.jsdelivr.net/npm/@hpcc-js/wasm-graphviz/dist/index.js").then(m => m.Graphviz); |
Copilot
AI
Jul 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The nested await import with chained .then() and ?? reduces readability. Consider splitting this into separate try/catch or sequential if blocks for clarity.
| const Graphviz = await import("./packages/graphviz/dist/index.js").then(m => m.Graphviz).catch(() => undefined) ?? | |
| await import("https://cdn.jsdelivr.net/npm/@hpcc-js/wasm-graphviz/dist/index.js").then(m => m.Graphviz); | |
| let Graphviz; | |
| try { | |
| const localModule = await import("./packages/graphviz/dist/index.js"); | |
| Graphviz = localModule.Graphviz; | |
| } catch (error) { | |
| console.warn("Failed to load local Graphviz module, falling back to remote module:", error); | |
| } | |
| if (!Graphviz) { | |
| const remoteModule = await import("https://cdn.jsdelivr.net/npm/@hpcc-js/wasm-graphviz/dist/index.js"); | |
| Graphviz = remoteModule.Graphviz; | |
| } |
No description provided.