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

xAxis Labels.formatter does not work if changed to ES6 style #8580

Closed
Scarittagle opened this issue Jul 5, 2018 · 7 comments
Closed

xAxis Labels.formatter does not work if changed to ES6 style #8580

Scarittagle opened this issue Jul 5, 2018 · 7 comments

Comments

@Scarittagle
Copy link

@Scarittagle Scarittagle commented Jul 5, 2018

Expected behaviour
So In ES5 the format of writing the labels.formatter is like this:

labels: { formatter: function() { //something..... } }

but if I changed to ES6 style:
labels: { formatter: () => { //something..... } }

It should render the label as usual.
Actual behaviour
Instead the chart stopped render all the xAxis labels.

Live demo with steps to reproduce

http://jsfiddle.net/tDSrd/291/

Product version

Highcharts 6.1.1

Affected browser(s)
Chrome Version 67.0.3396.87 (Official Build) (64-bit)

@sebastianbochan
Copy link
Contributor

@sebastianbochan sebastianbochan commented Jul 6, 2018

Hi @Scarittagle,
Thank you for reporting.

Internal note
I remember that we had similar case in the past, but not sure if it was fixed or not.
@TorsteinHonsi @jon-a-nygaard

@pawelfus
Copy link
Contributor

@pawelfus pawelfus commented Jul 6, 2018

@Scarittagle

This is caused how fat arrow functions deal with context. It's not only syntactic sugar, "ES6 style", but also this in fat arrow functions refers to the current scope context:

new Highcharts.chart('container', {
  ...
  formatter: function ()  {
    // this -> refers to the context passed on, in this case: "label"
  }
  ...
});
new Highcharts.chart('container', {
  ...
  formatter: () => {
    // this -> refers to the context where chart was created, in this case "document"
  }
  ...
});

Changed this in fat arrow functions is the reason why those exist :)

Edit:
Better example:

new Highcharts.chart('container', {
  chart: {
    events: {
      load: function () {
        setTimeout(() => {
          console.log(this); // this refers to "chart" as it's a context of the higher scope
        });
      }
    }
  },
  ...
new Highcharts.chart('container', {
  chart: {
    events: {
      load: function () {
        setTimeout(function () {
          console.log(this); // this refers to "window" as it's a context of anonymous function
        });
      }
    }
  },
  ...
});
@jon-a-nygaard
Copy link
Contributor

@jon-a-nygaard jon-a-nygaard commented Jul 31, 2018

As @pawelfus mentioned this is the expected result when using arrow function, and we can not go around it either by using call or apply because this is not binded to the function.

A possibility to consider though is providing the data on this as parameters instead to avoid future confusions. Unfortunately that will likely become a breaking change if carried out.

I'm not sure what would be the sensible action here. Do you have an opinion on this @TorsteinHonsi?

@TorsteinHonsi
Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi commented Aug 16, 2018

The params are currently not used for anything, so that makes sense. But we should keep the scope as is, and just add a parameter.

Demo with fix applied: http://jsfiddle.net/tDSrd/310/

@bdrazen
Copy link

@bdrazen bdrazen commented May 3, 2019

Can we also apply this update to the tooltip formatter?

@TorsteinHonsi
Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi commented May 6, 2019

Can we also apply this update to the tooltip formatter?

We could, but then we risk breaking backwards compatibility because the tooltip formatter currently takes the Tooltip instance as the first parameter.

@bdrazen
Copy link

@bdrazen bdrazen commented May 6, 2019

Ah. Second parameter maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants