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

repeat.for on Set leading to negative index #284

Closed
tkhyn opened this Issue Jun 14, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@tkhyn
Contributor

tkhyn commented Jun 14, 2017

I'm submitting a bug report

  • Library Version:
    1.4.0

Please tell us about your environment:

  • JSPM OR Webpack AND Version
    JSPM 0.17.b41

  • Browser:
    all

  • Language:
    ESNext

Current behavior:

When manipulating a Set bound to a repeat.for statement, an exception in aurelia-templating is raised when the Set becomes empty when manipulating its values.

It is highlighted in the following gist: https://gist.run/?id=ceda36a5c6481c2b629e7b117864c3c7

With files and line numbers we get:

aurelia-task-queue.js:58 Uncaught TypeError: Cannot read property 'firstChild' of undefined
    at ViewSlot.insert (aurelia-templating.js:1738)
    at Repeat.insertView (repeat.js:267)
    at SetRepeatStrategy.instanceMutated (set-repeat-strategy.js:60)
    at Repeat.handleCollectionMutated (repeat.js:171)
    at Repeat.call (repeat.js:115)
    at ModifySetObserver.callSubscribers (aurelia-binding.js:372)
    at ModifySetObserver.call (aurelia-binding.js:986)
    at TaskQueue.flushMicroTaskQueue (aurelia-task-queue.js:150)
    at MutationObserver.eval (aurelia-task-queue.js:78)

It is caused by the fact index equals -1 in ViewSlot.insert(index, view).

I suspect a Math.max(0, set.size - 1) could advantageously replace set.size - 1 in SetRepeatStrategy.instanceMutated but I have no idea of the side effects.

Expected/desired behavior:
No exception !

  • What is the motivation / use case for changing the behavior?
    The bound Set may be manipulated in several places of the program, resulting in the sequence (remove, add and then remove the same element) exposed in the Gist and crashing the program.
@tkhyn

This comment has been minimized.

Show comment
Hide comment
@tkhyn

tkhyn Jun 27, 2017

Contributor

@EisenbergEffect , @jdanyow and @martingust I'm having a go at this. I've already added a few basic tests for SetRepeatStrategy, but as ViewSlot is mocked the error cannot be reproduced in a test here.

Anyway, it looks like there are 2 ways to fix this:

  1. Either allow a negative index in ViewSlot.insert(index, view) by adding index = Math.max(0, index), that's a modification in aurelia/templating
  2. Or make sure that set.size - 1 is replaced by the value of Math.max(set.size - 1, 0) in SetRepeatStrategy.instanceMutated

In your opinion, where should the change occur ? I'd lean towards 2., I've tested it and it seems to work without any side effects.

Contributor

tkhyn commented Jun 27, 2017

@EisenbergEffect , @jdanyow and @martingust I'm having a go at this. I've already added a few basic tests for SetRepeatStrategy, but as ViewSlot is mocked the error cannot be reproduced in a test here.

Anyway, it looks like there are 2 ways to fix this:

  1. Either allow a negative index in ViewSlot.insert(index, view) by adding index = Math.max(0, index), that's a modification in aurelia/templating
  2. Or make sure that set.size - 1 is replaced by the value of Math.max(set.size - 1, 0) in SetRepeatStrategy.instanceMutated

In your opinion, where should the change occur ? I'd lean towards 2., I've tested it and it seems to work without any side effects.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Jun 27, 2017

Member

I need to defer to @jdanyow or @martingust on this.

Member

EisenbergEffect commented Jun 27, 2017

I need to defer to @jdanyow or @martingust on this.

tkhyn added a commit to tkhyn/templating-resources that referenced this issue Jun 30, 2017

@tkhyn

This comment has been minimized.

Show comment
Hide comment
@tkhyn

tkhyn Jun 30, 2017

Contributor

Thanks @EisenbergEffect.

@jdanyow, @martingust, PR #285 is live for discussions :)

Contributor

tkhyn commented Jun 30, 2017

Thanks @EisenbergEffect.

@jdanyow, @martingust, PR #285 is live for discussions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment