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

isFulfilledData should only consider value and not open #1526

Open
mozeryansky opened this issue Feb 26, 2024 · 6 comments · May be fixed by #1579
Open

isFulfilledData should only consider value and not open #1526

mozeryansky opened this issue Feb 26, 2024 · 6 comments · May be fixed by #1579

Comments

@mozeryansky
Copy link
Contributor

Lightweight Charts™ Version: latest

Following up from my question in discussions I encounter this as a bug often.

As Whitespace is a data item without a value from here: https://tradingview.github.io/lightweight-charts/docs/api/interfaces/WhitespaceData. However the function which I believe is where this is checked considers if open is also present and will throw and error: https://github.com/tradingview/lightweight-charts/blob/master/src/model/data-consumer.ts#L188

@SlicedSilver
Copy link
Contributor

It depends on the type of series that the data is intended for. The library expects one of the following:

  • 'Line type' data, which has value
  • or 'Bar type' data, which has open, high, low, close

As you can see there is no overlap of keys thus it is safe to check if the data is fulfilled by checking if either open or value are supplied. You shouldn't be supplying partially complete bars, or extra information in the data. If you do need to use extra info then make use of the customValues key.

@mozeryansky
Copy link
Contributor Author

mozeryansky commented Feb 27, 2024

I see that and I may not be using the API as intended, but it does seem like an unintended side effect. As I start with my candles and I compute my signal and add that as a the value. Then I pass the new data array as the argument for my line.

Psuedocode:

candles = getCandles('TSLA')
sma = addSMAValue(candles)

candleStickSeries.setData(candles)
lineSeries.setData(sma)

However, for the first values in the line series with undefined values (like sma_30) will error out saying "... series item data value of open must be a number ... " from here, since the validator will be true as (open OR value) is present.

Specifically, I had to change my code as follows:

// FROM: errors because `open` is present
const data = candles.map((bar, i) => {
    const value = smaValues[i - diff]
    return value ? { ...bar, value } : bar
})

// TO: when value is invalid, only include time
const data = candles.map((v, i) => {
    const value = smaValues[i - diff]
    return value ? { ...v, value } : { time: v.time }
})

I ask, because it seems it's a bug for line series to tell me open is causing an issue even though it never uses that value for anything and causes an exception to be thrown.


Edit: The assert is from the checkLineItem function here, which uses the same validation as checkBarItem.

@mozeryansky
Copy link
Contributor Author

I guess I'm just asking if there could instead be a isFulfilledBarData and isFulfilledLineData instead of only a isFulfilledData? I can make a PR if you think this is something to be considered.

@SlicedSilver
Copy link
Contributor

In principle it should be fine to add a different checker for each data type. It would probably only add a tiny bit to the bundle size and be easy to implement. So if you create a PR and the increase to the bundle size is tiny then we would consider merging it.

Whether it is required is still debatable because the types for LineData wouldn't actually let you have open or other extra properties. If you tried this in Typescript then you would get a compiler error. I know not everyone uses TS, so this issue wouldn't be known until runtime for vanilla JS developers.

The suggested approach would be to modify your addSMAValue function such that it returns only data containing time and value (optional) instead of adding value to existing bar data. You wouldn't want to be mutating the existing candles data anyway so if you are creating a new array for the sma values then it should also be easy to only include the expected properties.

Additionally, we actually already have a code sample for an SMA indicator on candles data here: https://jsfiddle.net/TradingView/537kjtfg/
It was written a few years ago, and you can ignore most of the code (which is displaying a legend, and generating random but realistic candle data), but you can pay attention to the calculateSMA function

@mozeryansky
Copy link
Contributor Author

I guess I've already written the code to avoid the issue so it's solved for me in a way, but when doing something quick I keep running into it. I use TS but I didn't see a compiler error, I'll look into that too. I should have time soon to try out a PR and see how the build size changes, it'll give me a better understanding of that portion of the code too. Thanks.

@mozeryansky
Copy link
Contributor Author

I've created a PR. Let me know what you think, thanks!

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