-
-
Couldn't load subscription status.
- Fork 8.9k
fix(runtime-core): warn about negative number in v-for #12308
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
Maybe it would be worth adding test cases for 0 too? It feels like something that could easily get broken without tests to catch it.
|
We could add test cases for |
|
Yes, I meant adding tests to confirm that Your code contains a boundary condition of |
d8bf594 to
d289f93
Compare
WalkthroughValidation logic for numeric sources in both Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant renderList/ssrRenderList
Caller->>renderList/ssrRenderList: Call with source (number)
alt source is not a positive integer
renderList/ssrRenderList->>Caller: Warn "expects a positive integer value"
renderList/ssrRenderList->>Caller: Return empty list
else source is a positive integer
renderList/ssrRenderList->>Caller: Return generated list
end
Estimated code review effort2 (~20 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
d289f93 to
7678a6c
Compare
|
Got it! I've updated the tests accordingly and pushed my changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
eff843f to
28c5ecb
Compare
Context
When using
v-forwith a negative value, let's say-1, an error occurs in the console:RangeError: Invalid array lengthYou can see a reproduction here.
Proposed solution
If the number wasn't an integer, we would get a nice warning:
I'm just extending this warning to also includes negative integers.
This way, it's way easier to understand that the issue arises in the
v-forand goes into debugging the issue faster.You can see a reproduction of the fix live here. (the console is correctly warning
[Vue warn]: The v-for range expects a positive integer value but got -1.)Side note
I did a commit for the
server-renderer, to stay aligned with theruntime-corechange.This one was failing silently as it's using a
for loopcompared to theruntime-coreusing anew Array(source)wheresourcewas-1, hence resulting in theRangeError: Invalid array lengtherror message.I did a second commit for the
runtime-corepackage, the one I want to fix.(actually this issue arised from using the carousel from NuxtUI, but I see that the page / indicators calculation has now been fixed but I still think that this fix is nice to have)
Summary by CodeRabbit
Bug Fixes
Tests