Added check that the ${each_block_value} exists

[petterek]

Also chaches the length of the array in a variable to avoid quering for it every time.
Does this fix #681 ?

[Rich-Harris]

Thanks for the PR. It depends on the behaviour we want — elsewhere, Svelte errs on the side of demanding explicitness rather than trying to guess what the developer meant, because that way it's easier to track down errors. For example I'm a Brit on an American team, which means misunderstandings like this aren't all that uncommon:

{{#each colours as colour}}
  <span style='color: {{colour}};'>{{colour}}</span>
{{/each}}

<script>
  export default {
    data () {
      return {
        colors: ['red', 'orange', 'yellow', 'green', 'blue', 'indigo', 'violet']
      };
    }
  };
</script>

Right now, that gives me an error — the proposal in #681 was to provide a more useful error if the component was compiled with dev: true. Without that error, we could be pulling our hair out for a long time before we realised what was wrong.

Incidentally, caching .length doesn't actually have any effect — that's something you get for free with modern JS engines.

[petterek]

I just observed that when using the Proxy to wrap an object the length was called every time.

[petterek]

At least I was able to create a failing test :)

[Rich-Harris]

I just observed that when using the Proxy to wrap an object the length was called every time.

I think that's just the Heisenberg effect — the property is accessed because there's a proxy

[petterek]

How should we handle such situation?

Uninitialized fields should

• throw exception that the field does not exist
• just ignore and loop out nothing
• any other?

[petterek]

I belived this was an easy fix :) I'm obviously not smart enough to fix it :(

[Rich-Harris]

I'm obviously not smart enough to fix it :(

Nonsense! It just failed because of a snapshot test. It's the correct fix if we want to prevent errors regardless of whether there's data for the each block. I'm inclined to argue against doing that for the reasons mentioned above, but I'm open to persuasion! If anyone else out there has thoughts on the matter, let us know.

[Conduitry]

I'd vote for not preventing the error. It's more in line with how other things in Svelte work, and it might help you catch typos or other bugs. With the other recent change (#683) it might not even be necessary to add a specific dev warning about this, but I feel less strongly about that.

[petterek]

I belive there are good reasons for having to "predefine" all fields/properties on your viewmodel.
And I agree with both of you that it can be very frustrating if you have such a case with misspelling, I've been there..

Maybe throw an exception with a good description about witch field is missing.

What if the field is there, but is null. Is this the same situation? Should the check be against the hasOwnProperty and don't evaluate the value?
In such case we would need to do the || [] not to go into an error state!

[petterek]

Template:
{{each Items as item}} <p>item.name</p>{{\each}}

If data looks like this it will throw exception
{"Prop1":123}
but if it looks like this it will not
{"Prop1":123, Items: null}

[petterek]

Should we keep this PR open @Rich-Harris or should I delete it?

Did we end up with a conclusion?

[Rich-Harris]

I think we should probably stick with the current behaviour — masking user errors is almost always a bad idea. If you compile with dev: true, it will check that properties have been initialised, which helps you pinpoint the source of the error:

(This is the REPL running locally, it doesn't actually use dev: true currently. Should probably fix that.)

So I'll close this PR. Thank you anyway!