Skip to content

CL-2954: Update datafeed tutorial#156

Open
anachuprina wants to merge 2 commits into
masterfrom
CL-2954-update-datafeed-tutorial
Open

CL-2954: Update datafeed tutorial#156
anachuprina wants to merge 2 commits into
masterfrom
CL-2954-update-datafeed-tutorial

Conversation

@anachuprina
Copy link
Copy Markdown
Contributor

No description provided.

@anachuprina anachuprina requested a review from SlicedSilver May 22, 2026 14:14
Copy link
Copy Markdown

@SimonMorda SimonMorda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment as Simon instead of Romain but didn't want to switch user and restart again.

Comment thread README.md
@@ -1,62 +1,120 @@
# Advanced Charts: Connecting data via the Datafeed API
# TradingView Charting Library Datafeed Example
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a mention somewhere when was the last time this tutorial was tested and with which versions of the TV lib and dependencies (aka Binance, CoinDesk, etc)?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the following:

Last tested: June 2026 with TradingView Advanced Charts 31.2.0, Trading
Platform 31.2.0, Binance public Spot REST/WebSocket APIs, CoinDesk RSS, and the
npm dependencies from package-lock.json.

@@ -0,0 +1,236 @@
import {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im aware you've explained in other md files the purpose of this file but could you at least add an intro at the very top of the file to quickly indicate what's that file for while inviting people to read the README & INTEGRATION files?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a short description/header

@@ -0,0 +1,162 @@
import { cp, mkdir, rm, stat } from 'node:fs/promises';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the same style of header here too

Comment thread src/trading.js Outdated
Comment on lines +178 to +181
const alertIndex = activeAlerts.findIndex(
alert => alert.id === id
);
if (alertIndex === -1) return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the setTimeout is very small I would move this block down into it. activeAlerts may be different between now and within the setTimeout resulting in out of date data.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used codex and moved the alert lookup inside the timeout and now resolve by id against the latest activeAlerts state before updating.

Comment thread src/trading.js Outdated

notify(
'Alert Triggered',
`Price crossed your alert at ${alert.price.toFixed(2)}`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for toFixed.

Comment thread src/trading.js Outdated
const price = lastPlusClickPrice;
const alertAction = actionsFactory.createAction({
actionId: 'create_custom_alert',
label: `Add Alert at ${price.toFixed(2)}`,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible refinement to bring here at some point.
Depending on the formater used/precision set in Settings, toFixed may be wrong.
Not the end of the world though.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex replaced toFixed with helped function that falls back to toFixed. I think this helped is not working so it will always fall back to toFixed(). I will look into it more in the future

Comment thread src/datafeed/helpers.js Outdated
Comment on lines +209 to +211
if (interval === '1M') {
return null;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@SimonMorda
Copy link
Copy Markdown

Really good tutorial!

I think some tweaks could be brought to centralize some common logic, especially when it comes to creating and managing web socket creation.
Let's keep that in mind for a future refinement.

@SimonMorda
Copy link
Copy Markdown

@illetid , @SlicedSilver could you also please provide your feedback? 🙏

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants