Conversation
codesankalp
force-pushed
the
fix/issue-392
branch
3 times, most recently
from
7b1d3ca to
5236b0e
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The main problem I see is that now yarn build produces multiple files, which doesn't make it easy for us to drop the resulting work in OpenWISP. Is this on purpose?
Yes, there are several reasons for this approach:
The contenthash in filenames will optimize CDN cache management when using cdn (if we are using cdn). Integration Considerations:
let me know your thoughts! |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@codesankalp what do you think about the following?
- a result set providing the current output: all separate files
- a result set providing the library + echarts
- a result set providing the library + echarts + leaflet
Can you add basic explanation about the above?
codesankalp
force-pushed
the
fix/issue-392
branch
4 times, most recently
from
68ed674 to
17ffba3
Compare
|
New Buid Sizes:
|
codesankalp
force-pushed
the
fix/issue-392
branch
2 times, most recently
from
e43988e to
5355c03
Compare
codesankalp
force-pushed
the
fix/issue-392
branch
2 times, most recently
from
0c99ec8 to
b4bbfa9
Compare
There was a problem hiding this comment.
I have the following problem:
yarn install
yarn install v1.22.22
[1/4] Resolving packages...
[2/4] Fetching packages...
error Error: https://registry.yarnpkg.com/error-ex/-/error-ex-1.3.3.tgz: Request failed "404 Not Found"
at ResponseError.ExtendableBuiltin (/usr/share/yarn/lib/cli.js:696:66)
at new ResponseError (/usr/share/yarn/lib/cli.js:802:124)
at Request.<anonymous> (/usr/share/yarn/lib/cli.js:66750:16)
at Request.emit (node:events:519:28)
at module.exports.Request.onRequestResponse (/usr/share/yarn/lib/cli.js:142287:10)
at ClientRequest.emit (node:events:519:28)
at HTTPParser.parserOnIncomingClient (node:_http_client:716:27)
at HTTPParser.parserOnHeadersComplete (node:_http_common:117:17)
at TLSSocket.socketOnData (node:_http_client:558:22)
at TLSSocket.emit (node:events:519:28)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
I see the following line in yarn.lock:
debug@4, debug@^4.1.0, debug@^4.1.1, debug@^4.3.1, debug@^4.3.2, debug@^4.3.4, debug@^4.4.1:
version "4.4.2"
resolved "https://registry.yarnpkg.com/debug/-/debug-4.4.2.tgz#96b480a7fe47cc04fe57f0bf56e058dba2a001fa"
integrity sha512-IQeXCZhGRpFiLI3MYlCGLjNssUBiE8G21RMyNH35KFsxIvhrMeh5jXuG82woDZrYX9pgqHs+GF5js2Ducn4y4A==
dependencies:
ms "^2.1.3"
The URL https://registry.yarnpkg.com/debug/-/debug-4.4.2.tgz#96b480a7fe47cc04fe57f0bf56e058dba2a001fa returns 404.
I am not sure why the CI build is not complaining about this, but it feels wrong to me to reference a 404 URL in yarn.lock, what do you think?
codesankalp
force-pushed
the
fix/issue-392
branch
from
b4bbfa9 to
4d026ce
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I did some changes to the README.
The save on the complete bundle is only ~300KB.
In OpenWISP dashboard we need to load the complete bundle, so reducing 300KB is good but I wonder if we really need 1.3 MB to show geographic and indoor maps.
Is there a way to reduce the imports of echarts so that we only import exactly what we use and let the javascript pruning tools do the work of removing all the JS code that we don't use?
codesankalp
force-pushed
the
fix/issue-392
branch
2 times, most recently
from
bc058f2 to
e6c4b28
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
added
the
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
index.html (1)
123-131:⚠️ Potential issue | 🟡 MinorMake mobile cards truly full-width by sizing the card container.
width: 100%on the<a>alone won’t necessarily expand the.cardsflex item, so the mobile layout may still hug content width. Consider setting the card width in the media query.Proposed tweak
`@media` (max-width: 768px) { main { flex-direction: column; align-items: center; } + .cards { + width: 100%; + max-width: 100%; + } + .cards a { width: 100%; } }.github/workflows/ci.yml (1)
26-29:⚠️ Potential issue | 🟡 MinorUpdate deprecated
set-outputcommand.The
::set-outputcommand is deprecated. Use$GITHUB_OUTPUTenvironment file instead.🔧 Suggested fix
- name: Get yarn cache directory path id: yarn-cache-dir-path - run: echo "::set-output name=dir::$(yarn cache dir)" + run: echo "dir=$(yarn cache dir)" >> $GITHUB_OUTPUTAlso applies to line 69.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 5-6: The package.json "main" entry points to a non-existent
index.js; fix by either adding a top-level index.js that re-exports the
library's public API (matching what built bundles export) or update the "main"
field to the actual build output (e.g., "dist/netjsongraph.js" or the production
filename pattern). Modify package.json "main" to the chosen bundle or create an
index.js that imports/exports from the build output (ensure symbols exported by
your library remain identical), and update any build/publish scripts if needed
so the referenced file exists after npm publish.
In `@webpack.config.js`:
- Around line 89-103: The optimization config sets sideEffects: false which
disables webpack's side-effects-based tree-shaking; change sideEffects to true
inside the optimization object (where usedExports, providedExports,
concatenateModules, etc. are set) so webpack will respect the package.json
"sideEffects" flags and drop unused modules—update the optimization.sideEffects
property from false to true and re-run the build to validate minification via
getMinimizers still behaves as expected.
🧹 Nitpick comments (1)
webpack-plugins/inject-leaflet-loader.js (1)
38-46: Consider extracting CDN version to a constant or reading from package.json.The Leaflet version
1.8.0is hardcoded in the CDN URLs. If the version inpackage.jsonis updated, these URLs would become stale. Consider extracting the version to a constant at the top of the file or reading it from package.json.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/ci.ymlindex.htmlknip.jsonpackage.jsonsrc/js/echarts-leaflet/index.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.jswebpack-plugins/inject-leaflet-loader.jswebpack.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/leaflet-loader.js
- src/js/netjsongraph.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
webpack.config.jsknip.jsonwebpack-plugins/inject-leaflet-loader.jssrc/js/echarts-leaflet/index.jspackage.json
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/echarts-leaflet/index.js
🧠 Learnings (8)
📓 Common learnings
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:51:06.213Z
Learning: In the netjsongraph.js project, imports from `echarts/lib/chart/*/install`, `echarts/lib/component/*/install`, and `echarts/lib/renderer/install*` paths produce better tree-shaking results than the public entry points (`echarts/charts`, `echarts/components`, `echarts/renderers`) in their webpack configuration. The project maintainer has tested both approaches and confirmed the `/lib/*/install` pattern yields smaller bundles.
📚 Learning: 2026-01-20T16:51:06.213Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:51:06.213Z
Learning: In the netjsongraph.js project, imports from `echarts/lib/chart/*/install`, `echarts/lib/component/*/install`, and `echarts/lib/renderer/install*` paths produce better tree-shaking results than the public entry points (`echarts/charts`, `echarts/components`, `echarts/renderers`) in their webpack configuration. The project maintainer has tested both approaches and confirmed the `/lib/*/install` pattern yields smaller bundles.
Applied to files:
webpack.config.jswebpack-plugins/inject-leaflet-loader.jspackage.json
📚 Learning: 2026-01-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
webpack-plugins/inject-leaflet-loader.js
📚 Learning: 2026-01-20T00:42:32.369Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:42:32.369Z
Learning: In the netjsongraph.js codebase, node IDs are guaranteed not to contain the "~" character, so using "source~target" as a composite key for links in nodeLinkIndex will not collide with node ID keys.
Applied to files:
webpack-plugins/inject-leaflet-loader.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/echarts-leaflet/index.js
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Applies to test/netjsongraph.browser.test.js : Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with `yarn test test/netjsongraph.browser.test.js`
Applied to files:
package.json.github/workflows/ci.yml
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Applies to **/*.test.{js,ts} : Write unit tests using Jest with jsdom and run with `yarn test`
Applied to files:
package.json.github/workflows/ci.yml
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Run QA checks using `./run-qa-checks` before committing code
Applied to files:
.github/workflows/ci.yml
🧬 Code graph analysis (2)
webpack.config.js (2)
test/netjsongraph.duplicateNodes.test.js (1)
require(1-1)test/netjsonWorker.test.js (1)
require(1-1)
src/js/echarts-leaflet/index.js (5)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-38)src/js/netjsongraph.config.js (1)
L(370-370)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/echarts-leaflet/LeafletModel.js (1)
extendLeafletModel(6-50)src/js/echarts-leaflet/LeafletView.js (1)
extendLeafletView(6-122)
🪛 Biome (2.3.13)
webpack-plugins/inject-leaflet-loader.js
[error] 142-142: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 GitHub Check: CodeQL
.github/workflows/ci.yml
[warning] 52-129: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
🔇 Additional comments (11)
index.html (1)
245-249: The "Nodes expand or fold" demo ships correctly.The
netjsongraph-nodeExpand.htmlfile exists at the referenced path (./examples/netjsongraph-nodeExpand.html), so the link will not 404. No changes needed.knip.json (1)
5-6: LGTM!The
ignoreDependenciesaddition forleafletis appropriate since it's now an optional peer dependency that's dynamically loaded at runtime. This prevents knip from incorrectly flagging it as unused..github/workflows/ci.yml (1)
119-121: LGTM!Browser tests running unconditionally across all matrix build types (
dev,prod,echarts-only) ensures the echarts-only build is properly exercised by CI, addressing the review concern about detecting future breakage.src/js/echarts-leaflet/index.js (2)
10-16: LGTM!The early return pattern with
getLeaflet(true)(suppressing errors) gracefully enables graph-only rendering when Leaflet is unavailable. This aligns well with the echarts-only build strategy.
1-5: LGTM!The imports are correctly structured. The
echarts/coreimport forregisterCoordinateSystemandregisterActionis appropriate since these are core registration APIs, not chart/component/renderer modules where the/lib/*/installpattern applies for tree-shaking. Based on learnings, the/lib/*/installpattern is specifically for chart, component, and renderer imports.webpack-plugins/inject-leaflet-loader.js (1)
57-127: LGTM!The plugin loading logic with
waitForLeafletpolling (5-second timeout) and coordinatedonload/onerrorhandlers ensures robust initialization. The error handlers appropriately prevent blocking initialization if a plugin fails to load.package.json (2)
89-99: LGTM!The dependency structure is now correct:
leafletindevDependenciesfor development/buildingleafletas optionalpeerDependencyfor consumers who need map features- Core
dependenciesreduced to onlyechartsandkdbushThis ensures echarts-only consumers won't install Leaflet by default while preserving full build capability.
6-6:sideEffects: falseis correctly configured and safe to use.The concern about import-time side effects breaking module registration is unfounded. The
registerLeafletSystem()function is imported but not called at module load time—it's explicitly invoked only wheninitializeECharts()runs during construction. The echarts registrations (registerCoordinateSystem,registerAction) occur inside the function body, not on import. No CSS imports or other import-time side effects exist that would be affected by tree-shaking.webpack.config.js (3)
61-88: LGTM!The build configuration cleanly handles both build variants:
isEchartsOnlyflag derived fromBUILD_TYPEenvironment variable- Conditional filenames with contenthash for production caching
- Leaflet externalized as
"L"for echarts-only builds, aligning with CDN loading
22-59: LGTM!The TerserPlugin configuration with aggressive compression options (3 passes, dead code elimination, unused removal) should produce well-optimized production bundles. The
hoist_vars: falsesetting is appropriate as hoisting variables can cause issues in some scenarios.
169-171: LGTM!The
DefinePlugincorrectly setsBUNDLE_LEAFLETtotruefor full builds andfalsefor echarts-only builds, aligning with the conditional logic insrc/js/leaflet-loader.js(lines 22-24) that decides whether torequire("leaflet")or usewindow.L.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
This pull request significantly reduces library bundle size by implementing conditional dependency bundling, removing unused features (echarts-gl, graphGL example, animation example), and creating two build variants to optimize for different deployment scenarios.
Changes:
- Introduced two build configurations: full bundle (with Leaflet) and echarts-only bundle (without Leaflet)
- Implemented dynamic Leaflet loading via CDN for echarts-only builds using a custom webpack plugin
- Removed echarts-gl dependency and associated examples (graphGL, map animation)
- Refactored ECharts imports to use tree-shakeable imports from echarts/core
- Updated CI/CD pipeline to test both build variants
Reviewed changes
Copilot reviewed 25 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Added support for two build types (full/echarts-only), optimized terser settings, added compression plugins, and conditional Leaflet externalization |
| webpack-plugins/inject-leaflet-loader.js | New plugin that dynamically injects Leaflet loader code into HTML files for echarts-only builds |
| src/js/leaflet-loader.js | New module providing conditional Leaflet loading (bundled vs. global window.L) |
| src/js/netjsongraph.render.js | Refactored to use tree-shakeable ECharts imports and conditional Leaflet loading |
| src/js/netjsongraph.js | Updated to use echarts/core imports and lazy Leaflet registration |
| src/js/echarts-leaflet/* | Updated to support conditional Leaflet loading and use echarts/core imports |
| src/js/netjsongraph.config.js | Made CRS property a lazy getter that loads Leaflet dynamically |
| package.json | Moved Leaflet to peerDependencies (optional), removed echarts-gl and zrender, added build scripts |
| test/* | Updated imports and added async handling for render() method |
| .github/workflows/ci.yml | Added matrix strategy to test dev, prod, and echarts-only builds |
| README.md | Added comprehensive documentation for both build variants |
| knip.json | Added leaflet to ignoreDependencies |
| index.html | Removed links to deleted examples |
| public/example_templates/* | Removed graphGL and animation examples |
| public/assets/data/airplaneRouteMap.json | Removed (no longer needed) |
| yarn.lock | Updated dependencies, removed echarts-gl, updated various packages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
codesankalp
force-pushed
the
fix/issue-392
branch
from
d86848f to
3516536
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The only remaining issue is the Leaflet plugins example breaking, here's a screenshot:
This is how the current example on https://openwisp.github.io/netjsongraph.js/examples/netjsonmap-plugins.html looks to me:
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@codesankalp I've added a browser test for the leaflet plugin example in #503 and proceeded to merge this branch with master to get the test run in the CI build here, which is now failing as expected, do you see the JS console error now?
Signed-off-by: Sankalp <sankalp.nex@gmail.com>
Signed-off-by: Sankalp <sankalp.nex@gmail.com>
Should maintain consistent return type (undefined instead of null)
- `InjectLeafletLoaderPlugin` assumes specific HTML structure that may break - Uses regex to remove/replace script tags which is fragile
- Update CI to use matrix strategy for 'dev' (unit+browser), 'prod' (browser), and 'echarts-only' (browser) builds. - Remove unsafe Terser optimizations to ensure stability. - Fix 'InjectLeafletLoaderPlugin' regex to prevent accidental removal of plugin scripts. - Use 'http-server' via npx in CI for serving production builds. Signed-off-by: Sankalp <sankalp.nex@gmail.com>
Signed-off-by: Sankalp <sankalp.nex@gmail.com>
Signed-off-by: Sankalp <sankalp.nex@gmail.com>
codesankalp
force-pushed
the
fix/issue-392
branch
from
d8082d5 to
7af27bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 5-7: The package.json currently points "main" to
dist/netjsongraph.min.js but builds are ignored by git and not included in the
published package; add a "prepare" script (e.g., add "prepare": "yarn
build:full" to the scripts object so npm/yarn runs the build on publish) and/or
add a "files" array (e.g., include "dist/") to package.json to ensure the dist/
directory is packaged; update the scripts and/or files keys accordingly so that
the "main" entry exists in the published package.
In `@src/js/netjsongraph.config.js`:
- Around line 366-375: The crs getter on config calls getLeaflet() without
suppressing warnings, causing repeated console warnings in echarts-only builds;
update the getter to call getLeaflet(true) so it suppresses the warning when
Leaflet is absent and keep the rest of the logic (return L ? L.CRS.EPSG3857 :
null) intact—modify the Object.defineProperty for config.crs that currently
references getLeaflet() to use getLeaflet(true) instead.
In `@webpack-plugins/inject-leaflet-loader.js`:
- Around line 62-70: The onerror handler for the created script element
(leafletJS) only logs to console and can leave the page stuck; update
leafletJS.onerror to both surface a visible error and recover: create or update
a DOM error element (e.g., show a banner/message) and then call initMap() (or
loadLeafletPlugins if hasPlugins is true) so the page proceeds in a degraded
state; locate the leafletJS creation block and modify leafletJS.onerror to
display an in-page error message and invoke the appropriate initializer (initMap
or loadLeafletPlugins) instead of only console.error.
In `@webpack.config.js`:
- Around line 207-211: The webpack performance config currently sets
maxEntrypointSize and maxAssetSize too low for the full build; update the
performance block (performance, maxEntrypointSize, maxAssetSize) to either raise
maxAssetSize (and optionally maxEntrypointSize) to values that cover the full
"Library + ECharts + Leaflet" bundle (e.g., bump maxAssetSize to ~450000 or
similar) or add a comment documenting that production builds will generate
warnings for the full-build entrypoint and those warnings are expected; change
the numeric thresholds in the performance config or add the explanatory comment
accordingly.
🧹 Nitpick comments (4)
src/js/netjsongraph.core.js (1)
80-80:async render()neverawaits the data-loading promise chain — consider returning or awaiting it.The method is marked
asyncbut thepaginatedDataParse.then()chain (Line 105) is fire-and-forget. This meansawait someInstance.render()resolves immediately, before data is loaded and the view is painted. If callers rely on the event system (onReady/onLoad) this works, but theasyncsignature is misleading — it suggests awaiting it guarantees a rendered state.Consider either:
return this.utils.paginatedDataParse.call(…).then(…)so the caller can actually await completion, or- Documenting clearly that render is fire-and-forget and callers should listen for lifecycle events.
src/js/leaflet-loader.js (1)
1-37: Clean runtime loader pattern — well structured.The caching, build-time branching via
BUNDLE_LEAFLET, and graceful fallback for the echarts-only build are solid. One tiny nit:Line 1's
/* eslint-disable import/no-mutable-exports */appears unnecessary — the only export is thegetLeafletfunction declaration (an immutable binding); the mutablelet Lis module-private and not exported.src/js/echarts-leaflet/LeafletCoordSys.js (1)
151-155: Dead code:typeof L === "undefined"can never be true here.
Lis a closure variable captured from line 12. SincecreateLeafletCoordSystem()throws on line 14 ifLis falsy, by the timeLeafletCoordSys.createexecutes,Lis guaranteed to be defined and truthy. This guard was meaningful whenLwas a global, but is now dead code.♻️ Suggested cleanup
if (leafletCoordSys) { throw new Error("Only one leaflet component can exist"); } - if (typeof L === "undefined") { - throw new Error("Leaflet api is not loaded"); - } if (!leafletModel.__map) {webpack.config.js (1)
186-202: Consider extractingzlibconstants to reduce repetition.
require("zlib")is called four times inline within the Brotli compression options. Extracting it to a local constant improves readability.♻️ Suggested refactor
+const zlib = require("zlib"); + // ...inside the production plugins array: new CompressionPlugin({ filename: "[path][base].br", algorithm: "brotliCompress", test: /\.(js|css|html|svg|json)$/, compressionOptions: { params: { - [require("zlib").constants.BROTLI_PARAM_QUALITY]: 11, - [require("zlib").constants.BROTLI_PARAM_SIZE_HINT]: 0, - [require("zlib").constants.BROTLI_PARAM_MODE]: - require("zlib").constants.BROTLI_MODE_TEXT, - [require("zlib").constants.BROTLI_PARAM_LGWIN]: 22, - [require("zlib").constants.BROTLI_PARAM_LGBLOCK]: 0, + [zlib.constants.BROTLI_PARAM_QUALITY]: 11, + [zlib.constants.BROTLI_PARAM_SIZE_HINT]: 0, + [zlib.constants.BROTLI_PARAM_MODE]: zlib.constants.BROTLI_MODE_TEXT, + [zlib.constants.BROTLI_PARAM_LGWIN]: 22, + [zlib.constants.BROTLI_PARAM_LGBLOCK]: 0, }, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
lib/css/assets/leaflet-measure.pngis excluded by!**/*.pngpublic/assets/images/floorplan.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.eslintrc.js.github/workflows/ci.ymlREADME.mdindex.htmlknip.jsonpackage.jsonpublic/assets/data/airplaneRouteMap.jsonpublic/assets/data/netjsongraph-graphGL.jsonpublic/example_templates/netjsongraph-graphGL.htmlpublic/example_templates/netjsonmap-animation.htmlpublic/example_templates/netjsonmap-indoormap.htmlsrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.clients.jssrc/js/netjsongraph.config.jssrc/js/netjsongraph.core.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.jssrc/js/netjsongraph.render.jstest/browser.test.utils.jstest/netjsongraph.duplicateNodes.test.jstest/netjsongraph.render.test.jswebpack-plugins/inject-leaflet-loader.jswebpack.config.js
💤 Files with no reviewable changes (4)
- test/browser.test.utils.js
- public/example_templates/netjsongraph-graphGL.html
- public/assets/data/airplaneRouteMap.json
- public/example_templates/netjsonmap-animation.html
🚧 Files skipped from review as they are similar to previous changes (5)
- src/js/netjsongraph.clients.js
- knip.json
- .eslintrc.js
- test/netjsongraph.duplicateNodes.test.js
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
webpack.config.jspackage.jsonwebpack-plugins/inject-leaflet-loader.jssrc/js/netjsongraph.jssrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/netjsongraph.config.jstest/netjsongraph.render.test.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.core.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.jssrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/netjsongraph.config.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.core.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.render.test.js
🧠 Learnings (11)
📓 Common learnings
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:51:06.213Z
Learning: In the netjsongraph.js project, imports from `echarts/lib/chart/*/install`, `echarts/lib/component/*/install`, and `echarts/lib/renderer/install*` paths produce better tree-shaking results than the public entry points (`echarts/charts`, `echarts/components`, `echarts/renderers`) in their webpack configuration. The project maintainer has tested both approaches and confirmed the `/lib/*/install` pattern yields smaller bundles.
📚 Learning: 2026-01-20T16:51:06.213Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:51:06.213Z
Learning: In the netjsongraph.js project, imports from `echarts/lib/chart/*/install`, `echarts/lib/component/*/install`, and `echarts/lib/renderer/install*` paths produce better tree-shaking results than the public entry points (`echarts/charts`, `echarts/components`, `echarts/renderers`) in their webpack configuration. The project maintainer has tested both approaches and confirmed the `/lib/*/install` pattern yields smaller bundles.
Applied to files:
webpack.config.jspackage.jsonwebpack-plugins/inject-leaflet-loader.jstest/netjsongraph.render.test.js
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Applies to test/netjsongraph.browser.test.js : Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with `yarn test test/netjsongraph.browser.test.js`
Applied to files:
package.jsontest/netjsongraph.render.test.js.github/workflows/ci.yml
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Applies to **/*.test.{js,ts} : Write unit tests using Jest with jsdom and run with `yarn test`
Applied to files:
package.json.github/workflows/ci.yml
📚 Learning: 2026-01-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
webpack-plugins/inject-leaflet-loader.jssrc/js/netjsongraph.render.js
📚 Learning: 2026-01-20T00:42:32.369Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:42:32.369Z
Learning: In the netjsongraph.js codebase, node IDs are guaranteed not to contain the "~" character, so using "source~target" as a composite key for links in nodeLinkIndex will not collide with node ID keys.
Applied to files:
webpack-plugins/inject-leaflet-loader.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.jssrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/netjsongraph.config.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.core.js
📚 Learning: 2026-01-27T19:09:23.929Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 473
File: src/js/netjsongraph.render.js:433-468
Timestamp: 2026-01-27T19:09:23.929Z
Learning: In the netjsongraph.js project, when implementing wheel zoom propagation from the graph container to the canvas: ECharts intercepts wheel events directly on the canvas with preventDefault(), so those events don't bubble to the container. A wheel listener on the container only catches events that occur outside the canvas (in the empty container space), making checks for e.target === canvas unnecessary since canvas events never reach the container listener.
Applied to files:
src/js/netjsongraph.jssrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.render.js
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
src/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.config.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.core.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Run QA checks using `./run-qa-checks` before committing code
Applied to files:
.github/workflows/ci.yml
🧬 Code graph analysis (7)
webpack.config.js (1)
webpack-plugins/inject-leaflet-loader.js (1)
path(5-5)
src/js/echarts-leaflet/LeafletCoordSys.js (2)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-37)src/js/echarts-leaflet/index.js (1)
L(11-11)
src/js/netjsongraph.geojson.js (4)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-37)src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/netjsongraph.config.js (1)
L(370-370)
src/js/echarts-leaflet/LeafletView.js (1)
src/js/echarts-leaflet/LeafletCoordSys.js (3)
mapOffset(126-126)mapOffset(132-132)api(113-113)
src/js/netjsongraph.config.js (4)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-37)src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/netjsongraph.geojson.js (2)
L(136-136)L(140-140)
test/netjsongraph.render.test.js (1)
test/netjsongraph.duplicateNodes.test.js (1)
NetJSONGraphRender(2-2)
src/js/leaflet-loader.js (4)
src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/netjsongraph.config.js (1)
L(370-370)src/js/netjsongraph.geojson.js (2)
L(136-136)L(140-140)
🪛 Biome (2.3.13)
webpack-plugins/inject-leaflet-loader.js
[error] 160-160: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 GitHub Check: CodeQL
.github/workflows/ci.yml
[warning] 52-129: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage (dev)
🔇 Additional comments (28)
.github/workflows/ci.yml (1)
83-97: CI matrix covers all build variants with browser tests — good setup.Conditional build/server steps cleanly separate the three paths, and browser tests (Line 121) run unconditionally across all matrix entries, which matches the reviewer request to exercise echarts-only builds in CI.
src/js/echarts-leaflet/LeafletView.js (1)
1-7: Clean refactor to directecharts/coreimports and parameterless factory.The shift from dependency-injected
echarts/Lparameters to direct imports fromecharts/coreis consistent across all echarts-leaflet modules and removes unnecessary coupling. Theconsttightening onpartsandmapOffsetis a nice touch.src/js/echarts-leaflet/LeafletModel.js (1)
1-7: Consistent refactor mirroring the LeafletView.js pattern — LGTM.Direct import of
extendComponentModelfromecharts/coreand parameterless factory signature align with the other echarts-leaflet module changes.Also applies to: 17-17
src/js/netjsongraph.render.js (4)
1-26: Modular echarts imports via/lib/*/installpaths — consistent with project learnings.The
use()registration pattern with individual install imports aligns with the tested approach that yields smaller bundles in this project's webpack configuration. Addition ofCanvasRendereralongsideSVGRendererensures both renderers are available. Based on learnings, the/lib/*/installpattern produces better tree-shaking results than the public entry points in this project's webpack configuration.
515-520: Runtime Leaflet resolution with fail-fast error — solid approach.Destructuring
{circleMarker, latLngBounds}from the loaded Leaflet instance at the top ofmapRenderkeeps the rest of the method clean and avoids repeatedL.references. The early throw ensures a clear error message when Leaflet is genuinely missing.
206-207: Simplified to always usetype: "graph"— completes the graphGL removal.Hardcoding
type: "graph"with a"force"layout fallback removes the last graphGL code path. The|| "force"default is a safe choice for the standard graph layout.
871-871: Narrowed public API to a single default export.Removing the named
echartsandLexports in favor ofexport default NetJSONGraphRenderis a clean API surface reduction. Consumers that need echarts or Leaflet directly should useecharts/coreor the leaflet-loader respectively.webpack-plugins/inject-leaflet-loader.js (1)
1-21: Build-time integrity computation looks good.The approach of reading the Leaflet version and computing SRI hashes from the local
node_modulescopy at build time is solid — it ensures the CDN-served assets match what was tested locally.public/example_templates/netjsonmap-indoormap.html (1)
13-17: LGTM — Leaflet JS CDN tag added alongside existing CSS.The added script tag correctly matches the existing Leaflet CSS version (1.8.0) and includes an SRI integrity hash. The
InjectLeafletLoaderPluginwill strip these static references during echarts-only builds and replace them with the dynamic loader, while full builds will overridewindow.Lwith the bundled copy.src/js/netjsongraph.geojson.js (1)
1-1: Clean migration to runtime Leaflet loading.The pattern is consistent with other modules:
getLeaflet()→ early return if unavailable → destructure needed API. ThegeoJSONfactory destructuring fromLis correct for Leaflet 1.8.0.Also applies to: 136-140, 167-167
test/netjsongraph.render.test.js (3)
1-3: Import changes align with the new default-export pattern.
Lis now imported directly from"leaflet"andNetJSONGraphRenderuses a default import, consistent with the refactored module exports.
477-481: Async render test properly awaits the now-asyncrender().The switch from a synchronous throw expectation to
await map.render()+console.errorassertion correctly reflects the new async render path.
707-715: Good addition: prototype isolation prevents test pollution.The
beforeEach/afterEachsnapshot-and-restore ofNetJSONGraphRender.prototype's prototype chain prevents theObject.setPrototypeOfcall insetUp(line 718) from leaking across tests.package.json (1)
90-101: Dependency restructuring looks correct.Leaflet as an optional peer dependency, echarts and kdbush as direct dependencies — this properly supports both build variants.
zrenderremoval is fine since it's a transitive dependency ofecharts.src/js/netjsongraph.js (2)
8-11: Conditional Leaflet bundling for full builds.The
BUNDLE_LEAFLETguard correctly ensures Leaflet is onlyrequired and assigned towindow.Lin the full build variant. This runs before any component initialization, sogetLeaflet()calls in downstream modules will find it.
176-178: Default export added alongsidewindow.NetJSONGraph.This properly supports both ESM bundler consumers (via
import) and script-tag consumers (viawindow.NetJSONGraph).src/js/echarts-leaflet/LeafletCoordSys.js (2)
11-24: Clean refactoring to loader-based Leaflet access.The parameterless factory that obtains Leaflet via
getLeaflet()and destructures the needed APIs is well-structured. Thethrowon line 14 ensures no code path proceeds without a valid Leaflet instance.
175-197: Destructured Leaflet API usage is correct.
lMap,lTileLayer, andcontrol.layersproperly replace the formerL.map,L.tileLayer, andL.control.layerscalls. The aliased names (lMap,lTileLayer) avoid conflicts with local variables.src/js/netjsongraph.config.js (2)
1-1: LGTM on the import change.Clean switch from direct Leaflet import to the runtime loader pattern, consistent with the rest of the codebase.
377-378: Named + default exports look good.Exporting
prepareDatafrom the original config object (not the spread copy) preserves the original reference, and the default export is the enhanced config with the dynamiccrsgetter.webpack.config.js (5)
22-59: Aggressive Terser settings look appropriate for a production library bundle.The multi-pass compression with
pure_getters,collapse_vars, andtoplevelis well-suited for reducing the final output.hoist_vars: falseavoids potential issues with variable hoisting across modules.
89-103:sideEffects: trueis correctly set now—good.This properly enables webpack's side-effects optimization, consistent with the past review feedback.
83-88: Leaflet externalization for echarts-only build is correctly configured.Mapping
leafletto the globalLensures Leaflet is excluded from the bundle while remaining available at runtime viawindow.L.
169-171:DefinePluginforBUNDLE_LEAFLETcorrectly drives the runtime loader.
JSON.stringify(!isEchartsOnly)ensures the flag is properly injected as a boolean literal, which theleaflet-loader.jsmodule checks at runtime.
68-81: The review comment is incorrect. Thebuildscript in package.json is defined as"yarn build:full"only, not as a combined script that runs bothbuild:fullandbuild:echarts-onlysequentially. The two build variants are separate scripts that must be invoked independently. Whileclean: truewould erase the first build's output if both scripts were run one after another manually, this is not an automated workflow issue in the current package.json configuration.Likely an incorrect or invalid review comment.
src/js/echarts-leaflet/index.js (3)
1-5: Imports are well-structured.Using
echarts/corefor the registration APIs (registerCoordinateSystem,registerAction) is correct—the tree-shaking learning aboutecharts/lib/*/installpaths applies to chart/component/renderer modules, not the core registration surface.
10-16: Clean early-exit for missing Leaflet.
getLeaflet(true)correctly suppresses the warning, and bailing out before any Leaflet-dependent registration ensures the echarts-only build works without errors. This is the right pattern.
18-36: Registration flow looks correct.The parameterless calls to
extendLeafletModel(),extendLeafletView(), andcreateLeafletCoordSystem()align with the refactored modules that now source their dependencies internally. TheregisterActioncallback logic is unchanged.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "main": "dist/netjsongraph.min.js", | ||
| "module": "src/js/netjsongraph.js", | ||
| "sideEffects": true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== .gitignore entries for dist ==="
grep -n "dist" .gitignore 2>/dev/null || echo "No .gitignore or no 'dist' entry"
echo ""
echo "=== .npmignore existence and content ==="
cat .npmignore 2>/dev/null || echo "No .npmignore file"
echo ""
echo "=== package.json 'files' field ==="
grep -A 10 '"files"' package.json 2>/dev/null || echo "No 'files' field in package.json"
echo ""
echo "=== package.json build-related scripts ==="
grep -E '"(build|prepublish|prepack|prepare)"' package.json 2>/dev/null || echo "No build/publish scripts found"
echo ""
echo "=== Full package.json scripts section ==="
grep -A 20 '"scripts"' package.json | head -30Repository: openwisp/netjsongraph.js
Length of output: 1127
🏁 Script executed:
#!/bin/bash
echo "=== GitHub Actions workflows ==="
find .github/workflows -name "*.yml" -o -name "*.yaml" 2>/dev/null | while read f; do
echo "File: $f"
grep -E "(publish|npm|release|dist)" "$f" 2>/dev/null | head -5
echo ""
done || echo "No workflows directory found"
echo ""
echo "=== .npmrc file ==="
cat .npmrc 2>/dev/null || echo "No .npmrc file"
echo ""
echo "=== Build output directory structure ==="
ls -la dist/ 2>/dev/null || echo "dist/ directory does not exist (expected - it's gitignored)"Repository: openwisp/netjsongraph.js
Length of output: 389
🏁 Script executed:
#!/bin/bash
echo "=== Full package.json to verify all scripts ==="
jq '.scripts' package.json 2>/dev/null || cat package.json | grep -A 50 '"scripts"'
echo ""
echo "=== CI workflow file content ==="
head -100 .github/workflows/ci.ymlRepository: openwisp/netjsongraph.js
Length of output: 3292
Add a prepare script and/or files field to ensure dist/ is included in the npm package.
The main field points to dist/netjsongraph.min.js, but dist/ is in .gitignore with no files field or .npmignore to override it. Without a prepare script (e.g., "prepare": "yarn build:full"), the build artifact will not be generated during npm publish, and the published package will be missing its main entry point. Add either "prepare": "yarn build:full" to scripts and/or a "files" field to package.json to fix this.
🤖 Prompt for AI Agents
In `@package.json` around lines 5 - 7, The package.json currently points "main" to
dist/netjsongraph.min.js but builds are ignored by git and not included in the
published package; add a "prepare" script (e.g., add "prepare": "yarn
build:full" to the scripts object so npm/yarn runs the build on publish) and/or
add a "files" array (e.g., include "dist/") to package.json to ensure the dist/
directory is packaged; update the scripts and/or files keys accordingly so that
the "main" entry exists in the published package.
| const config = {...NetJSONGraphDefaultConfig}; | ||
|
|
||
| Object.defineProperty(config, "crs", { | ||
| get() { | ||
| const L = getLeaflet(); | ||
| return L ? L.CRS.EPSG3857 : null; | ||
| }, | ||
| enumerable: true, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
getLeaflet() in the crs getter should suppress the warning.
The getter calls getLeaflet() without suppressError, so in echarts-only builds (where Leaflet is intentionally absent) every access to config.crs will emit a console warning. Unlike one-shot calls, a getter can be triggered repeatedly—by spread operations, serialization, or any code path that reads the config. Other call sites that tolerate a missing Leaflet (e.g., src/js/echarts-leaflet/index.js line 11) already pass true.
🔧 Suggested fix
Object.defineProperty(config, "crs", {
get() {
- const L = getLeaflet();
+ const L = getLeaflet(true);
return L ? L.CRS.EPSG3857 : null;
},
enumerable: true,
configurable: true,
});🤖 Prompt for AI Agents
In `@src/js/netjsongraph.config.js` around lines 366 - 375, The crs getter on
config calls getLeaflet() without suppressing warnings, causing repeated console
warnings in echarts-only builds; update the getter to call getLeaflet(true) so
it suppresses the warning when Leaflet is absent and keep the rest of the logic
(return L ? L.CRS.EPSG3857 : null) intact—modify the Object.defineProperty for
config.crs that currently references getLeaflet() to use getLeaflet(true)
instead.
| const leafletJS = document.createElement('script'); | ||
| leafletJS.src = 'https://unpkg.com/leaflet@${LEAFLET_VERSION}/dist/leaflet.js'; | ||
| leafletJS.integrity = '${LEAFLET_JS_INTEGRITY}'; | ||
| leafletJS.crossOrigin = ''; | ||
| leafletJS.onerror = function() { | ||
| console.error('Failed to load Leaflet from CDN. Please check your internet connection.'); | ||
| }; | ||
| leafletJS.onload = ${hasPlugins ? "loadLeafletPlugins" : "initMap"}; | ||
| document.head.appendChild(leafletJS); |
There was a problem hiding this comment.
Leaflet CDN load failure silently prevents page initialization.
When the Leaflet JS script fails to load from CDN (line 66-68), onerror only logs to the console but never calls initMap(). The page will silently hang with no visual indication. Consider at minimum adding a visible error message or, if a degraded experience is acceptable, calling initMap() anyway (which will then surface the missing L at usage sites).
🔧 Suggested minimal fix — surface the failure
leafletJS.onerror = function() {
console.error('Failed to load Leaflet from CDN. Please check your internet connection.');
+ document.body.innerHTML = '<p style="color:red;padding:1em;">Failed to load Leaflet. Check your connection and reload.</p>';
};🤖 Prompt for AI Agents
In `@webpack-plugins/inject-leaflet-loader.js` around lines 62 - 70, The onerror
handler for the created script element (leafletJS) only logs to console and can
leave the page stuck; update leafletJS.onerror to both surface a visible error
and recover: create or update a DOM error element (e.g., show a banner/message)
and then call initMap() (or loadLeafletPlugins if hasPlugins is true) so the
page proceeds in a degraded state; locate the leafletJS creation block and
modify leafletJS.onerror to display an in-page error message and invoke the
appropriate initializer (initMap or loadLeafletPlugins) instead of only
console.error.
| performance: { | ||
| hints: isProduction ? "warning" : false, | ||
| maxEntrypointSize: 400000, | ||
| maxAssetSize: 300000, | ||
| }, |
There was a problem hiding this comment.
Performance thresholds may trigger warnings for the full build.
The PR reports "Library + ECharts + Leaflet: 366 KB", which exceeds maxEntrypointSize: 400000 (≈390 KB) only narrowly and exceeds maxAssetSize: 300000 (≈293 KB). If the full-build entrypoint is a single file, expect webpack warnings in production. Consider raising maxAssetSize slightly or acknowledging these warnings are expected for the full build.
🤖 Prompt for AI Agents
In `@webpack.config.js` around lines 207 - 211, The webpack performance config
currently sets maxEntrypointSize and maxAssetSize too low for the full build;
update the performance block (performance, maxEntrypointSize, maxAssetSize) to
either raise maxAssetSize (and optionally maxEntrypointSize) to values that
cover the full "Library + ECharts + Leaflet" bundle (e.g., bump maxAssetSize to
~450000 or similar) or add a comment documenting that production builds will
generate warnings for the full-build entrypoint and those warnings are expected;
change the numeric thresholds in the performance config or add the explanatory
comment accordingly.
I thought you were facing issues in echarts build, and i was testing that only. After making the change, all the tests are passing. |
2 participants
Checklist
Reference to Existing Issue
Closes #392
Description of Changes
Use chunk splitting, dynamic loading and removed unused examples to reduce library size.
#425 (comment)