openwisp / netjsongraph.js

NetJSON NetworkGraph visualizer.
http://netjson.org
BSD 3-Clause "New" or "Revised" License
270 stars 79 forks source link

[bug] Geographic map is broken after updating dependencies #244

Open nemesifier opened 8 months ago

nemesifier commented 8 months ago

I did some manual testing after merging https://github.com/openwisp/netjsongraph.js/pull/243 and it turns out the geographic map is now broken, see below for the error details.

We need to add a basic browser test which is able to detect this failure so we're protected from similar errors in the future. If we cannot find a solution quickly we'll need to rollback the recent changes.

Screenshot from 2024-02-27 16-47-06

log.js:70 [ECharts] Series scatter is used but not imported.
import { ScatterChart } from 'echarts/charts';
echarts.use([ScatterChart]);
outputLog @ log.js:70
Show 1 more frame
Show less
netjsongraph.core.js:108 TypeError: Cannot read properties of undefined (reading 'getProgressive')
    at eval (Scheduler.js:157:37)
    at GlobalModel.eval (Global.js:582:10)
    at Array.forEach (<anonymous>)
    at each (util.js:259:13)
    at GlobalModel.eachSeries (Global.js:580:67)
    at Scheduler.restorePipelines (Scheduler.js:156:13)
    at prepare (echarts.js:1070:17)
    at ECharts.setOption (echarts.js:480:9)
    at NetJSONGraphRender.echartsSetOption (netjsongraph.render.js:81:1)
    at Object.mapRender [as render] (netjsongraph.render.js:319:1)
eval @ netjsongraph.core.js:108
Shiva953 commented 8 months ago

@nemesifier Actually it was related to the echarts dependency. In the latest stable version of echarts, one would need to import scatter chart from the echarts/charts and then use it in echarts.use([ScatterChart]) to load and initialize the ScatterChart component.

Inside netjsongraph.render.js:

// other imports
import { ScatterChart } from 'echarts/charts'; // this needs to be imported separately

class NetJSONGraphRender {
        echartsSetOption(customOption, self) {
                  // defining configs, echartsLayer and commonOption
                  echarts.use([ScatterChart]); // necessary use
                  // ...
       }
// ...
}

With this change, the geographic map works fine now:

Screenshot 2024-02-28 at 4 35 56 PM

Speaking of general tests for detecting these kinds of errors,

  1. What should be a typical example(s) to start testing these on? In this one particularly, they are related to the individual dependency itself rather than a problematic code snippet which expects a different output.
  2. Should a different file be made(ex : netjsongraph.browser.test.js OR it should be tested in the individual test files for each module(netjsongraph.render.test.js in this case)?

I can make a PR for a short term fix of the geographic map one if you want.

d1vyanshu-kumar commented 8 months ago
Screenshot 2024-02-28 at 7 39 04 PM

@Shiva953 Great it works!! but could you please check your code, after changing the code that you mentioned the map does not look like as previously the dots were missing at the end in my OS.

nemesifier commented 8 months ago
  1. What should be a typical example(s) to start testing these on? In this one particularly, they are related to the individual dependency itself rather than a problematic code snippet which expects a different output.

A typical test is opening the page and verifying that some elements that we expect to be there is really there. The test should fail if there's a bug so we try causing the bug on purpose to ensure the test fails, in this case we don't need to do it on purpose because the bug is already there.

  1. Should a different file be made(ex : netjsongraph.browser.test.js OR it should be tested in the individual test files for each module(netjsongraph.render.test.js in this case)?

A new file in this case is better, as the file grows we split it into multiple files to avoid working with huge files.

We have JS browser tests in openwisp-wifi-login-pages, here it should be easier to run these type of tests because there's no server side app needed.

I can make a PR for a short term fix of the geographic map one if you want.

Sure!