fix(recharts): support function vars for axis tick formatters - #7366
harsh21234i wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 4 files
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Merging this PR will not alter performance
Comparing Footnotes
|
…formatter' into fix/7257-recharts-function-tick-formatter
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| elif tick_formatter is not None: | ||
| raise TypeError(_TICK_FORMATTER_TYPE_ERROR) |
There was a problem hiding this comment.
Please remove this branch. It rejects a plain rx.Var with type Any, and that input works on main.
XAxis.create(tick_formatter=rx.Var("((v) => v)")) renders tickFormatter:((v) => v) on main. On this branch it raises TypeError: tick_formatter must be a FunctionVar. A user can use rx.Var("myFormatter") to point at a JS function from custom code, so this breaks working apps. Please also add a test for this case.
There was a problem hiding this comment.
idk about this suggestion. a Var could be anything, we don't want to take anything for this prop. The point of strong type hinting is that it makes it harder to represent a bad state. So while a subset of plain Var might work, there's a lot of Var objects that won't work and will give weird frontend errors as runtime.
I think it makes sense to enforce that this field is a plain literal string, or explicitly a FunctionVar
| elif isinstance(tick_formatter, FunctionVar): | ||
| props["tick_formatter"] = tick_formatter | ||
| elif isinstance(tick_formatter, Var) and tick_formatter._var_type is str: | ||
| raise TypeError(_TICK_FORMATTER_DYNAMIC_VAR_ERROR) | ||
| elif callable(tick_formatter) and not isinstance(tick_formatter, Var): | ||
| raise TypeError(_TICK_FORMATTER_CALLABLE_ERROR) |
There was a problem hiding this comment.
Please remove the FunctionVar branch and the callable branch. The framework already does both of these checks.
- The
FunctionVarbranch puts back the value that is already inprops. The new annotation acceptsFunctionVar, sosuper().create()lets it through. super().create()already raisesTypeErrorfor a Python lambda onmain.
With only the str-Var check kept, FunctionStringVar, .partial(), ArgsFunctionOperation, a literal string and an Any Var all render correctly. A State str var is still rejected. _TICK_FORMATTER_CALLABLE_ERROR and _TICK_FORMATTER_TYPE_ERROR are then unused and can go too.
| elif isinstance(tick_formatter, Var) and tick_formatter._var_type is str: | ||
| raise TypeError(_TICK_FORMATTER_DYNAMIC_VAR_ERROR) |
There was a problem hiding this comment.
create() now raises TypeError here. Please add a Raises: section to the Axis.create docstring. The summary line talks only about string formatters, so please update it to describe function vars too.
| @@ -0,0 +1,53 @@ | |||
| import pytest | |||
There was a problem hiding this comment.
Please move these tests into tests/units/components/recharts/test_cartesian.py. That file already has the tick_formatter tests, including the tests that reject non-callables and Python callables.
FarhanAliRaza
left a comment
There was a problem hiding this comment.
I tested this branch in a small Reflex app with four line charts. Each chart uses a different tick_formatter: a literal string, a FunctionStringVar, a FunctionStringVar.partial(State.suffix), and an ArgsFunctionOperation.
All four charts render the formatted ticks. A click that changes State.suffix updates the partial formatter's ticks. The browser console shows no errors.
I also called XAxis.create() with the same inputs on main and on this branch. On main, a FunctionStringVar raises the TypeError from #7257, and this branch fixes that. A State str var is now rejected, as the issue asks. A plain rx.Var with type Any works on main but raises on this branch.
The requested changes are in the inline comments.
Summary
FunctionStringVar.Testing
scripts/make_pyi.py --checkpassed.git diff --checkpassed.Fixes #7257