100's of js errors - Error: <rect> attribute width: A negative value is not valid

[ollyboy]

When resizing windows using chrome developer tools we get lots of javascript errors. To me this is a show stopper for production use of charts

[ollyboy]

From this code ( minified ) code ie n.setAttribute(i, a)

function createSVG(t, e) {
    var n = document.createElementNS("http://www.w3.org/2000/svg", t);
    for (var i in e) {
        var a = e[i];
        if ("inside" === i)
            $$1(a).appendChild(n);
        else if ("around" === i) {
            var r = $$1(a);
            r.parentNode.insertBefore(n, r),
            n.appendChild(r)
        } else
            "styles" === i ? "object" === (void 0 === a ? "undefined" : _typeof$2(a)) && Object.keys(a).map(function(t) {
                n.style[t] = a[t]
            }) : ("className" === i && (i = "class"),
            "innerHTML" === i ? n.textContent = a : n.setAttribute(i, a))
    }
    return n

[scmmishra]

Can you share a code-pen or codesandbox ?

[ollyboy]

the code is exact copy of the sample given as you can see by the chart rendered on the right side of chrome in Dev mode. The issue I think only occurs when the browser window is reduced in size. The JavaScript needs to not call setattribute for width of object with negative number. Assume something odd in the scaling logic gives a negative width

[scmmishra]

Assume something odd in the scaling logic gives a negative width

Yes, you are right. I finally managed to replicate this. The following code in BaseChart makes the resizing on window size change

https://github.com/frappe/charts/blob/136b9142dcf46bd8d9a72b88f8b52f93a04b2199/src/js/charts/BaseChart.js#L85-L99

The draw function recalculates the width of the chart here

https://github.com/frappe/charts/blob/136b9142dcf46bd8d9a72b88f8b52f93a04b2199/src/js/charts/BaseChart.js#L136-L138

As it turns out the width that's being returned is negative. Which cascades down to calcXPositions in AxisChart which then follows into the chart components causing the issue.

I replicated this is while working on Frappe Framework desk, there are pages and a sidebar to toggle them. When a page with a sidebar is hidden and the window is resized, this error occurs. Probably because it's not visible. I'll have to investigate this further.

The most straightforward way it to destroy the event listener when the chart is not required, but in most cases this is not practical.

If we look at the code in dom.js

https://github.com/frappe/charts/blob/136b9142dcf46bd8d9a72b88f8b52f93a04b2199/src/js/utils/dom.js#L69-L75

The right and left padding is added to the overall width. Having a negative padding can be a cause of the problem too. Does your element have any negative padding?

UPDATE

Turns out the getElementContentWidth function returns zero. However, the getExtraWidth function returns a positive value which is subtracted from the baseWidth and the number becomes negative.

https://github.com/frappe/charts/blob/136b9142dcf46bd8d9a72b88f8b52f93a04b2199/src/js/charts/BaseChart.js#L161

Need to figure out why the width is zero in your case. Are you sure that's the only chart your have created on that page? Also can try running the getElementContentWidth function on your console for the chart? You can pass a JS dom element and it should work.

[scmmishra]

I've made a new release hopefully fixing the bug at least for the hidden item issue I faced. Here's the commit https://github.com/frappe/charts/commit/d3eabea83612c1f8549826d3a6c6798abab424a3.

I've released the fix and published it on npm.

@ollyboy can you please check if this fixes the bug

[ollyboy]

ok will review this fix via npm

[ollyboy]

did npm and bench update, can see new version on charts loaded. Still getting same errors, assume npm install is not conflicting with some basic fappe/erpnext chart package. Maybe way to go is to protect the calls from negative values and console log a helpful error message

[scmmishra]

Maybe way to go is to protect the calls from negative values and console log a helpful error message

Yeah, perhaps we have to do this. I was avoiding this, trying to solve the problem at the root. But then, it makes sense to have the safety there since the rect can never be negative width.

On another note, the screenshot on your issue description is a frappe page, is it possible for you to send me the JS code for that page? Including the page controllers?

[scmmishra]

@ollyboy I managed to replicate a lot of these issues and have fixed them in a recent release. Can you please verify the fix

[ollyboy]

Code I am using. Reads a doc and displays a chart on a frappe built page. code is pasted into the .js that frappe generates for each new page.

frappe.pages['project-charts'].on_page_load = function(wrapper) {

   var page = frappe.ui.make_app_page({
                parent: wrapper,
                title: 'Project Charts',
                single_column: true
        });

    // hidden div 
    wrapper = $(wrapper).find('.layout-main-section');
    wrapper.append(`<div id="my_chart"></div>`);

    // define empty arrays for chart call
    var y_axis = [];
    var plan_capex = [];
    var actual_capex = [];
    var plan_fte = [];
    var actual_fte = [];

    // read a data set from a summary doc
    frappe.call({
      method:"frappe.client.get_list",
      args: {
        doctype:"Finance Resource forcast",
            filters: [ ],
        fields:["csfr_week_end", "csfr_plan_capex", "csfr_actual_capex", "csfr_plan_fte" , "csfr_actual_fte"  ]
            },

      callback: function(data) {

            // build arrays suitable to pass to charts
            for (let key in data) {
                let value = data[key];
                for (i = 0; i < value.length; i++) {
                        plan_capex.push(value[i].csfr_plan_capex || 0 );
                        actual_capex.push(value[i].csfr_actual_capex || 0 );
                        plan_fte.push(value[i].csfr_plan_fte || 0);
                        actual_fte.push(value[i].csfr_actual_fte || 0 );
                        y_axis.push(value[i].csfr_week_end || 0 );
                        //console.log ( value[i].csfr_week_end );
                }
              }
       }
    });

    let chart = new frappe.Chart( "#my_chart", { // or DOM element
        data: {

                labels: y_axis,

        datasets: [
                {
                        name: "Plan Capex", chartType: 'bar',
                        values: plan_capex
                },
                {
                        name: "Actual Capex", chartType: 'bar',
                        values: actual_capex
                },
                {
                        name: "Plan FTEs", chartType: 'line',
                        values: plan_fte
                },
                {
                        name: "Actual FTEs", chartType: 'line',
                        values: actual_fte
                }
        ],

        //yMarkers: [{ label: "Marker", value: 70,
        //      options: { labelPos: 'left' }}],
        //yRegions: [{ label: "Region", start: -10, end: 50,
        //      options: { labelPos: 'right' }}]
        },

        title: "Finance Resource forcast",
        type: 'axis-mixed', // or 'bar', 'line', 'pie', 'percentage'
        height: 300,
        colors: ['purple', '#ffa3ef', 'light-blue', 'green'],

        tooltipOptions: {
                formatTooltipX: d => (d + '').toUpperCase(),
                formatTooltipY: d => d + ' pts',
        }
  });

  // hacks to make the hidden div visible
  setTimeout(function(){ window.dispatchEvent(new Event('resize')); }, 100);

}

[ollyboy]

Results from this code. Note I have taken all hooks.py references to the chart library out. Frappe seems to find the npm module. Checked and the version is 1.5.2. Stiil getting errors from:

"innerHTML" === i ? n.textContent = a : n.setAttribute(i, a))

How do I get frappe to use the non minified charts js for testing debug ?? I dont know how frappe is finding and including the npm installed js and css??

[scmmishra]

A lot of these errors are fixed in recent commits https://github.com/frappe/charts/commit/222cbb686f9e202cd7ad23aac24a490cf349f990, https://github.com/frappe/charts/commit/773f93c4141aff1998627cff4b34954af766197f and https://github.com/frappe/charts/commit/9d03d502d9a166cf9e5fadd7d063f4f7dbee40e7

It's released you can check it out.

How do I get frappe to use the non minified charts js for testing debug There is the link command.

Here are the steps

1. Clone frappe charts anywhere in your filesystem and cd into the directory
2. Run yarn or npm install
3. Run yarn link
4. Go to frappe repo in the app directory of your bench folder
5. Run yarn link frappe-charts
6. Delete the node_modules folder and run bench setup requirements --node

This will link frappe charts to the local version

To activate live reloading, you need to do that following

1. Go to frappe charts folder
2. Run yarn run dev
3. Go to your bench directory, run bench start

With this, bench will rebuild the library with every change in frappe charts. You'll get live reloading.

[ollyboy]

Did this:

yarn add frappe-charts yarn list ( shows 1.5.2 installed ) cd into the newly installed frappe-charts folder Run yarn link ( says registered ) Go to frappe repo in the app directory of your bench folder Run yarn link frappe-charts ( says linked ) Delete the node_modules folder run bench setup requirements --node ( recreates the node_modules )

Did a yarn list in the frappe folder and it still says version 1.3.0

Still getting heaps of errors, don't think it's using the local package

BTW: I am running frappe in nginx production mode on a google compute ubuntu server

[ollyboy]

Can we clarify how to use local version 1.5.2, followed instructions but frappe still used the charts 1.3 native to frappe. Also - Why not update the frappe version?

[scmmishra]

I am running frappe in nginx production mode on a google compute ubuntu server

In this case I'd recommend not run the local development version. You can unlink using yarn unlink frappe-charts in the frappe app folder

In production mode, live reloading of packages is not available. You should upgrade the package using npm normally

[ollyboy]

OK did a npm update in frappe folder and a bench update and that gave me:

├── [email protected] -> /home/mcd_steven/frappe-bench/node_modules/frappe-charts

So the link did work, now I get the following, many errors have gone but some remain.

[scmmishra]

The fix made previously would just work on bar graphs and not path and circle. I just have to add the validation on these components and it'll start working. Anyway this means that the fix is working without disruption of any functionality

For now, you can continue working with charts as is. In later releases I shall fix it for other SVG components as well.

[ollyboy]

OK, the yarn environment looks a bit messy. is that an issue? Should this be a new frappe issue...

mcd_steven@erp-next:~/frappe-bench/apps/frappe$ yarn upgrade yarn upgrade v1.16.0 warning ../../package.json: No license field [1/4] Resolving packages... warning quagga > get-pixels > [email protected]: request has been deprecated, see https://github.com/request/request/issues/3142 warning babel-runtime > [email protected]: core-js@<3 is no longer maintained and not recommended for usage due to the number of issues. Please, upgrade your dependencies to the actual version of core-js@3. warning [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-commonjs. warning [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-multi-entry. warning cypress > [email protected]: request has been deprecated, see https://github.com/request/request/issues/3142 warning cypress > extract-zip > [email protected]: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.) warning node-sass > [email protected]: request has been deprecated, see https://github.com/request/request/issues/3142 warning node-sass > node-gyp > [email protected]: request has been deprecated, see https://github.com/request/request/issues/3142 warning [email protected]: This module has been deprecated and is no longer maintained. Please use @rollup/plugin-buble. warning rollup-plugin-buble > buble > [email protected]: This is not needed anymore. Use require('os').homedir() instead. warning [email protected]: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-node-resolve. [2/4] Fetching packages... error [email protected]: The engine "node" is incompatible with this module. Expected version ">=10". Got "8.16.0" error Found incompatible module. info Visit https://yarnpkg.com/en/docs/cli/upgrade for documentation about this command.

mcd_steven@erp-next:~/frappe-bench/apps/frappe$ yarn install yarn install v1.16.0 warning ../../package.json: No license field [1/4] Resolving packages... success Already up-to-date. Done in 0.70s.

mcd_steven@erp-next:~/frappe-bench/apps/frappe$ yarn check yarn check v1.16.0 warning ../../package.json: No license field warning "rollup-plugin-vue#@vue/component-compiler#postcss@>=6.0" could be deduped from "7.0.14" to "[email protected]" error "rollup-plugin-vue#@vue/component-compiler#@vue/component-compiler-utils#postcss" not installed error Found 1 errors. info Visit https://yarnpkg.com/en/docs/cli/check for documentation about this command.

mcd_steven@erp-next:~/frappe-bench/apps/frappe$ npm list | grep ERR npm ERR! peer dep missing: [email protected] - 3, required by [email protected] npm ERR! peer dep missing: popper.js@^1.16.0, required by [email protected] npm ERR! extraneous: [email protected] /home/mcd_steven/frappe-bench/apps/frappe/node_modules/parchment npm ERR! missing: parchment@github:quilljs/parchment#487850f7eb030a6c4e750ba809e58b09444e0bdb, required by [email protected]

[Manviel]

Any updates?

[casesolved-co-uk]

Here are the steps

1. Clone frappe charts anywhere in your filesystem and cd into the directory
2. Run yarn or npm install
3. Run yarn link
4. Go to frappe repo in the app directory of your bench folder
5. Run yarn link frappe-charts
6. Delete the node_modules folder and run bench setup requirements --node

This will link frappe charts to the local version

To activate live reloading, you need to do that following

1. Go to frappe charts folder
2. Run yarn run dev
3. Go to your bench directory, run bench start

With this, bench will rebuild the library with every change in frappe charts. You'll get live reloading.

I don't think this works anymore with the new frappe v14 build system. Are there any new instructions?