Skip to content
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

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

Open
nemesifier opened this issue Feb 27, 2024 · 3 comments
Open

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

nemesifier opened this issue Feb 27, 2024 · 3 comments
Labels

Comments

@nemesifier
Copy link
Member

I did some manual testing after merging #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
@nemesifier nemesifier added the bug label Feb 27, 2024
@nemesifier nemesifier changed the title [bug] Map is broken after updating dependencies [bug] Geographic map is broken after updating dependencies Feb 27, 2024
@Shiva953
Copy link
Contributor

Shiva953 commented Feb 28, 2024

@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
Copy link

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
Copy link
Member Author

  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!

Shiva953 added a commit to Shiva953/netjsongraph.js that referenced this issue Feb 29, 2024
Fixes the breaking geographic map caused as a result of updating the
    dependencies, especially the echarts one.

Fixes openwisp#244
@nemesifier nemesifier moved this to To do (general) in OpenWISP Contributor's Board Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: To do (general)
Development

No branches or pull requests

3 participants