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

fix(runtime-core): warn about negative number in v-for #12308

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cyrilf
Copy link

@cyrilf cyrilf commented Nov 1, 2024

Context

When using v-for with a negative value, let's say -1, an error occurs in the console:

RangeError: Invalid array length
image

You can see a reproduction here.


Proposed solution

If the number wasn't an integer, we would get a nice warning:

image

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-for and 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 the runtime-core change.
    This one was failing silently as it's using a for loop compared to the runtime-core using a new Array(source) where source was -1, hence resulting in the RangeError: Invalid array length error message.

  • I did a second commit for the runtime-core package, 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)

@edison1105 edison1105 added ready to merge The PR is ready to be merged. 🧹 p1-chore Priority 1: this doesn't change code behavior. labels Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior. ready to merge The PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants