ember-cli-chart icon indicating copy to clipboard operation
ember-cli-chart copied to clipboard

Add `chart-chartjs-financial` dependency resolution fix

Open walter opened this issue 4 years ago • 11 comments

Closes #127

Supersedes PR #136

Relies on #138

This fixes the chart-chartjs-financial import by replacing the unreleased as a packagechart-chartjs-financial dependency with temp-chartjs-candlestick which is simply a released version of the same code.

Normally I wouldn't advise using a fork exactly likely this, but currently ember-cli-chartjs is broken without fixing this dependency.

Thanks @devotox for his work on these two PRs and his patience!

walter avatar Oct 11 '21 01:10 walter

@walter When i try and use this I get an error that says

Uncaught (in promise) Error: "candlestick" is not a registered controller.

devotox avatar Oct 11 '21 10:10 devotox

@walter When i try and use this I get an error that says

Uncaught (in promise) Error: "candlestick" is not a registered controller.

I see it called in the dummy app application template, but without actually data. So hard to tell if it's functional.

Can you give some code context of how it's being called and some example data, please?

walter avatar Oct 11 '21 18:10 walter

Oh, one other note. When I was testing this add-on from within another app at first it wasn't picking up the changes as it was based on the same repo/branch as previously. I had to blow away yarn.lock and node_modules within that app to get it to pull in changes.

The upshot, if you are testing from an app confirm that the changes have been pulled in by looking in node_modules/ember-cli-chart/package.json and confirm that templ-chartjs-candlestick is in there.

walter avatar Oct 11 '21 18:10 walter

@devotox - never my request for context and code, I managed to recreate error in the test dummy app by passing in datasets in @data. Looking into it.

walter avatar Oct 11 '21 19:10 walter

@walter I was thinking maybe it was different versions of chart.js being used in ember-cli-chart and temp-chartjs-candlestick.

I am pretty sure I used the same versions at 3.5.1 but I may be wrong

One thing I havent tried that crossed my mind maybe using resolutions in package.json

devotox avatar Oct 11 '21 20:10 devotox

@devotox Yeah, it's weird. It looks like they both are using chart.js 3.5.1, so not a mismatch there. Also it looks like the controller is registered with chart_js.Chart.register(CandlestickController, OhlcController, CandlestickElement, OhlcElement); , but yeah, not resolving.

I wonder if has to do with ember-auto-import resolution. Looking into that now.

walter avatar Oct 11 '21 20:10 walter

@walter could it be that we are importing the index.esm.js file which does not actually register the controllers

Yup just tested it out. What we need to do is

import { CandlestickController, OhlcController, CandlestickElement, OhlcElement } from 'temp-chartjs-candlestick'; // packaged version of chart-chartjs-financial

Chart.register(CandlestickController, OhlcController, CandlestickElement, OhlcElement);

devotox avatar Oct 12 '21 12:10 devotox

Also in index.js I think all we need is this

'use strict';

module.exports = {
  name: require('./package').name,
};

Since we are no longer including any files

devotox avatar Oct 12 '21 12:10 devotox

@devotox thanks for getting that working (I think). I now have the candlestick chart rendering without errors, but I don't have test candlestick or OHLC chart data to verify they work with. Can you try them out locally and let me know if they work for you?

walter avatar Oct 12 '21 23:10 walter

@walter yup tested with ohlc and it works

devotox avatar Oct 14 '21 11:10 devotox

@walter yup tested with ohlc and it works

I've added a WIP commit that reverts to using chart-charts-financial now that it has a published release. I'm able to see charts in the dummy app, but don't have data to test with. Could you grab the latest and see if your ohlc stuff works with it @devotox?

walter avatar Nov 07 '21 22:11 walter