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

charts/src/js/charts/BaseChart.js

Lines 85 to 99 in 136b914

	configure() {
	let height = this.argHeight;
	this.baseHeight = height;
	this.height = height - getExtraHeight(this.measures);
	// Bind window events
	this.boundDrawFn = () => this.draw(true);
	window.addEventListener('resize', this.boundDrawFn);
	window.addEventListener('orientationchange', this.boundDrawFn);
	}
	destroy() {
	window.removeEventListener('resize', this.boundDrawFn);
	window.removeEventListener('orientationchange', this.boundDrawFn);
	}

The draw function recalculates the width of the chart here

charts/src/js/charts/BaseChart.js

Lines 136 to 138 in 136b914

	draw(onlyWidthChange=false, init=false) {
	this.updateWidth();

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

charts/src/js/utils/dom.js

Lines 69 to 75 in 136b914

	export function getElementContentWidth(element) {
	var styles = window.getComputedStyle(element);
	var padding = parseFloat(styles.paddingLeft) +
	parseFloat(styles.paddingRight);
	return element.clientWidth - padding;
	}

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.

charts/src/js/charts/BaseChart.js

Line 161 in 136b914

this.width = this.baseWidth - getExtraWidth(this.measures);

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 d3eabea.

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 222cbb6, 773f93c and 9d03d50

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:

├── frappe-charts@1.5.2 -> /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 > request@2.88.2: request has been deprecated, see request/request#3142
warning babel-runtime > core-js@2.6.11: 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 rollup-plugin-commonjs@8.4.1: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-commonjs.
warning rollup-plugin-multi-entry@2.1.0: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-multi-entry.
warning cypress > request@2.88.0: request has been deprecated, see request/request#3142
warning cypress > extract-zip > mkdirp@0.5.1: 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 > request@2.88.2: request has been deprecated, see request/request#3142
warning node-sass > node-gyp > request@2.88.2: request has been deprecated, see request/request#3142
warning rollup-plugin-buble@0.19.8: This module has been deprecated and is no longer maintained. Please use @rollup/plugin-buble.
warning rollup-plugin-buble > buble > os-homedir@2.0.0: This is not needed anymore. Use require('os').homedir() instead.
warning rollup-plugin-node-resolve@4.2.4: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-node-resolve.
[2/4] Fetching packages...
error rollup-plugin-postcss@2.9.0: 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 "postcss@7.0.14"
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: jquery@1.9.1 - 3, required by bootstrap@4.5.0
npm ERR! peer dep missing: popper.js@^1.16.0, required by bootstrap@4.5.0
npm ERR! extraneous: parchment@2.0.0-dev /home/mcd_steven/frappe-bench/apps/frappe/node_modules/parchment
npm ERR! missing: parchment@github:quilljs/parchment#487850f7eb030a6c4e750ba809e58b09444e0bdb, required by quill@2.0.0-dev.2

[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?