Skip to content

Commit

Permalink
Add missing string handler in v-for (#4499)
Browse files Browse the repository at this point in the history
Fix #4497
  • Loading branch information
posva authored and yyx990803 committed Dec 16, 2016
1 parent 34333ca commit 974247f
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/core/instance/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ export function renderMixin (Vue: Class<Component>) {
render: () => VNode
): ?Array<VNode> {
let ret: ?Array<VNode>, i, l, keys, key
if (Array.isArray(val)) {
if (Array.isArray(val) || typeof val === 'string') {
ret = new Array(val.length)
for (i = 0, l = val.length; i < l; i++) {
ret[i] = render(val[i], i)
Expand Down
18 changes: 18 additions & 0 deletions test/unit/features/directives/for.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -428,4 +428,22 @@ describe('Directive v-for', () => {
expect(vm.$el.textContent).toMatch(/\s+foo\s+bar\s+/)
}).then(done)
})

it('strings', done => {
const vm = new Vue({
data: {
text: 'foo'
},
template: `
<div>
<span v-for="letter in text">{{ letter }}.</span
</div>
`
}).$mount()
expect(vm.$el.textContent).toMatch('f.o.o.')
vm.text += 'bar'
waitForUpdate(() => {
expect(vm.$el.textContent).toMatch('f.o.o.b.a.r.')
}).then(done)
})
})

6 comments on commit 974247f

@bfanger
Copy link

Choose a reason for hiding this comment

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

This is going to hang sites when a backend returns html(large string) instead of the expected json array.

Converting a string to an array is easy in javascript <span v-for="letter in text.split('')">.
Is the convenience worth the risk?

Maybe introduce a Vue.options.directives.for.maxStringLength?
Infinity for the current behaviour, 0 to disable and a number to limit the iterators to for example 1024.

@LinusBorg
Copy link
Member

Choose a reason for hiding this comment

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

This is going to hang sites when a backend returns html(large string) instead of the expected json array.

If your API sends you HTML instead of expected JSON your problem is with your API, not with the rendering.

So this:

Maybe introduce a Vue.options.directives.for.maxStringLength?
Infinity for the current behavior, 0 to disable and a number to limit the iterators to for example 1024.

..would only heal the symptom, not the cause, so I don't see a real necessity for it.

@bfanger
Copy link

Choose a reason for hiding this comment

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

your problem is with your API

When you're developing a backend, errors like:
Warning: Cannot modify header information - headers already sent by... are not uncommon and followed by the echo json_encode($myArray); cause large strings. When this hangs the browser it's not a pleasant developer experience.

would only heal the symptom, not the cause

Agree, and having to remember to set an option to 0 for every project is not something i want to do either.


Luckily the issue can be prevented.

Backend tip
Frameworks like Laravel, which will send 500 Internal server for small (warning/notice level) mistakes, this prevents the response being handled by javascript.

Frontend tip
Use an ajax library that allow you to enforce the responsetype to json, this also prevents the response from being handled.

@posva
Copy link
Member Author

@posva posva commented on 974247f Dec 25, 2016

Choose a reason for hiding this comment

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

@bfanger Abouth the split method: I tested multiple ways of doing it and the for loop is the fastest way

@bfanger
Copy link

Choose a reason for hiding this comment

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

Sure, iterating over a string is faster than converting it to an array and then iterating over that. (as it needs more cpu and more memory).

But take https://jsonplaceholder.typicode.com/users of example, a small 5.6kb json file with 10 users.
When that file had an error before sending the json header, the v-for would be creating 5600 instances of the dom structure instead 10 (1 loop for every byte)
The massive amount of generated dom is what causing the browser to hang.

@posva
Copy link
Member Author

@posva posva commented on 974247f Dec 25, 2016

Choose a reason for hiding this comment

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

It doesn't seem to hang: https://jsfiddle.net/posva/at54mdyu/
The front should handle errors and make sure the data is passed correctly. Imagine you forgot to apply a filter to a query on the back but everything else is right. Your front is really going to hang because you are creating 1M rows instead of just 5 😄 . There's nothing Vue can do about this

Please sign in to comment.