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

Undocumented (and maybe unwanted) breaking changes in 3.0.0-beta.11 #8470

Closed
jnizet opened this issue Feb 21, 2021 · 9 comments · Fixed by #8480
Closed

Undocumented (and maybe unwanted) breaking changes in 3.0.0-beta.11 #8470

jnizet opened this issue Feb 21, 2021 · 9 comments · Fixed by #8480

Comments

@jnizet
Copy link

jnizet commented Feb 21, 2021

Expected Behavior

These lines of Typescript code, which compiled fine in 3.0.0-beta.10, should also compile in beta.11 (I haven't found any documented breaking change documenting those):

  const chartOptions: ChartConfiguration<'bar'> = {
      type: 'bar',
      // irrelevant properties omitted
      options: {
        plugins: {
          tooltip: {
            callbacks: {
              // error here because dataPoint doesn't exist anymore
              label: context => context.dataset.label + ': ' + formatNumber(context.dataPoint.y)
            }
          }
        },
        scales: {
          y: {
            ticks: {
              // error here because the tick value is now typed as unknown
              callback: (value: number) => formatNumber(value)
            }
          }
        }
      }
    };

Current Behavior

They don't compile anymore.

Possible Solution

I've had to change them to

  const chartOptions: ChartConfiguration<'bar'> = {
      type: 'bar',
      // irrelevant properties omitted
      options: {
        plugins: {
          tooltip: {
            callbacks: {
              label: context =>
                context.dataset.label +
                ': ' +
                formatNumber(context.dataset.data[context.dataIndex] as number)
                // dataPoint was easier to use than the above, and its `y` value was typed as number, whereas context.dataset.data[context.dataIndex] as number is typed as unknown
            }
          }
        },
        scales: {
          y: {
            ticks: {
              callback: (value) => formatNumber(value as number)
            }
          }
        }
      }
    };
@LeeLenaleee
Copy link
Collaborator

label: context => context.dataset.label + ': ' + formatNumber(context.dataPoint.y)
dataPoint has been renamed to parsed

@etimberg
Copy link
Member

Thanks @LeeLenaleee. I updated the release notes and PR to correctly tag it as a change.

@jnizet
Copy link
Author

jnizet commented Feb 21, 2021

Thanks. parsed is also typed as unknown though.

@etimberg
Copy link
Member

I will look into improving unknown there. I think the format is always { x: any, y: any } but before committing I want to see how x/y change with different scales and chart types

@jnizet
Copy link
Author

jnizet commented Feb 21, 2021

You of course know much better about the library than I do, and thus probably have a good rationale for the choice of the name parsed, but as a user of the library who tried to discover the API through auto-completion in the IDE, I found dataPoint clearer than parsed.

@etimberg
Copy link
Member

The reason we called it parsed was because it's an internally generated value that is created after we've parsed the data object. So internally it was called parsed. and then for simplicity the scriptable contexts were updated to include it as well and so we figured it was good to have a single name.

If we get lots of feedback on the discoverability of it, I'm happy to alias back to the old name as well. It's a pretty lightweight change.

@etimberg
Copy link
Member

Ok, so I did some research. The format changes for radar/polar area/doughnut charts.

Bar / Line Charts

parsed: { x: number; y: number }

Bubble charts

parsed: { x: number; y: number, _custom: number }

Radar / Polar Area Charts

parsed: { r: number }

Pie / Doughnut Charts

parsed: number

Possible Solution

I'd like to get some opinions on this proposal to fix it.

  1. On the DatasetController replace protected getParsed(index: number): AnyObject; with protected getParsed(index: number): TParsedData
  2. Add TParsedData to dataset definitions and provide correct defaults for each chart type
  3. Update the TooltipItem interface such that parsed: TParsedData
  4. Update the scriptable context to take TParsedData and change parsed to TParsedData[]

Assuming there are no implementation hiccups, I think this would work and correctly type it.

@kurkle
Copy link
Member

kurkle commented Feb 21, 2021

To me that sounds good.

@kurkle
Copy link
Member

kurkle commented Feb 21, 2021

Also note that float bars have a _custom of:

  item._custom = {
    barStart,
    barEnd,
    start: startValue,
    end: endValue,
    min,
    max
  };

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

Successfully merging a pull request may close this issue.

4 participants