chore: fail test for each-in not accessing props properly whose keys contain a period #17535
Conversation
[ci skip]
(cherry picked from commit 2b58f7a)
[BUGFIX beta] Fix substate interactions with aborts
(cherry picked from commit a968069)
[ci skip]
[ci skip] (cherry picked from commit dd7a199)
This fixes emberjs#17243 by only removing dependent keys in `teardown`. Since emberjs#16978 coalesces the work to add dependent keys to happen at most once, it is now incorrect to eagerly remove them in `didUnwatch` because that could happen for a variety of reasons while there are still other interested parties (see attached test cases). This limits the work of removing dependent keys to `teardown` only. The logic is that, while we want to defer setting up the "link" to the target property as lazily as possible, it is less important to "unlink" it eagerly once the work is done. It _may_ be possible to accomplish this, but the current amount of book-keeping is not sufficient to track the count properly. In our tests, we have created sequences like this: 1. setup (peekWatching = 0, so nothing happens) 2. get (consumed here) 3. willWatch (already consumed, no-op) 4. get (already consumed, no-op) 5. didUnwatch 6. ... In this case, it would be incorrect to "unlink" at step 5. As of PR emberjs#16978, `CONSUMED` is essentially a boolean flag, so it is not sufficient to track the balance, and also, there is no counterpart to `get`, which makes eager "unlinking" impossible without much more book-keeping. It's unclear that it would be worthwhile to do that. On the other hand, if we only "unlink" on teardown, this ensures that we are only "unlinking" at most once, and it is guaranteed that there are no longer any interested parties. Fixes emberjs#17243 Co-authored-by: Godfrey Chan <godfreykfc@gmail.com> (cherry picked from commit 1f8218b)
Don't remove dep keys in `didUnwatch`
Fixes a few issues: * Usage of positional arguments with custom components. * Forwarding attributes via `...attributes` to a dynamic component. * Prevent errors when rendering many template blocks (`Error: Operand over 16-bits. Got 65536`). Full changes here: glimmerjs/glimmer-vm@v0.37.0...v0.37.1 (cherry picked from commit 7600d7b)
…nstead of String.prototype extensions (cherry picked from commit 37d69f1)
(cherry picked from commit 58f3bec)
[ci skip]
jasonmit
changed the title
|
probably caused by #17166, I think offending line is https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/metal/lib/property_get.ts#L99-L101 |
|
We should add some tests if that is actually a thing we intend to support, but to my knowledge we have never "really" supported |
|
@rwjblue I haven't found anything detailing this getter path behavior either, so it may have been a bug this app was depending on for a few years. If we're not supporting it, I'd still like to send over tests so the behavior stays consistent into the future. In our case it was a bit trolly since the keys were not symbolic of a path - they were ISO datetime strings. |
|
Ya, I totally agree that we should nail down the specific desired behavior with tests. If for nothing more than to ensure we don’t regress and change behavior again. |
|
I'm going to go ahead and close this -- this was unspecified behavior and not an area we want to touch going forward. |
Related to #17529