Skip to content

Conversation

@itay-grudev
Copy link

While debugging an issue I stumbled upon this line and correct me if I'm wrong, but it looks like a typo.

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2025

⚠️ No Changeset found

Latest commit: 7e4a8aa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
layerchart ✅ Ready (View Log) Visit Preview 7e4a8aa

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 23, 2025

Open in StackBlitz

npm i https://pkg.pr.new/layerchart@670

commit: 7e4a8aa

@techniq
Copy link
Owner

techniq commented Oct 23, 2025

Hey @itay-grudev 👋. Thanks for the PR. What is the issue you are encountering?

ctx.config.* are the explicit props pass to <Chart> including <Chart xRange={...}>. By using ctx.config.xRange instead ctx.xRange it would not including any default xRange which in this case would be [0, width] (for radial we use [0, 2 * Math.PI])

I poked around on the PR preview and didn't see any change (better or worse) which was a little surprising.

@itay-grudev
Copy link
Author

@techniq I'm still reverse-engineering to figure it out, but I got TypeError: Cannot read properties of undefined (reading 'xRange'). I'm not sure layerchart is the issue yet.

That being said, I just thought it's strange that we check whether ctx.config.xRange is truthy, but then we use ctx.xRange.

Copy link
Owner

@techniq techniq left a comment

Choose a reason for hiding this comment

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

@itay-grudev awe, I see what you're saying (sorry I'm just starting my day and not caffeinated 🤣)

While there are cases where you want to known if a setting is user configured, I can't think of a reasons here.

I checked the blame history just to be safe and this change looks to have originated from the massive Svelte 5 migration so it very likely was an oversight.

Thanks for the PR!

@techniq techniq merged commit bc97cf8 into techniq:next Oct 23, 2025
4 checks passed
@itay-grudev itay-grudev deleted the next branch October 23, 2025 12:17
@itay-grudev
Copy link
Author

itay-grudev commented Oct 23, 2025

(sorry I'm just starting my day and not caffeinated 🤣)

I'm sorry about bothering you that early. Let me get you that coffee.

And thanks for the help.

@techniq
Copy link
Owner

techniq commented Oct 23, 2025

You're very generous. Thank you 🫶

@techniq
Copy link
Owner

techniq commented Oct 23, 2025

btw, forgot to mention, but in the future it's best to attach a changeset to a pr (pnpm changeset) so when it is merged it will be added to the CHANGELOG and trigger the CI pipeline to cut a release to npm (after merging the release PR which accumulated these changesets).

This being a minor change with no known change might not warrant it, but always nice to capture for tracing purposes (especially if we encounter an unexpected regression).

Note: We're in the process of improving state management throughout LayerChart that will enable a lot of features and also improve testability. We also have a massive docs overhaul in progress :)

@techniq techniq mentioned this pull request Oct 24, 2025
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.

2 participants