Changes to buffered proxy not appearing in template

achambers commented 3 years ago

Hi folks. Got a small bug when upgrading my Ember app. Interestingly, this bug was attempted to be fixed previously, and that "fix" seems to now be a bug. More detail below:

What I expected to see

When modifying a property on a buffer proxy object, via an input field, the changes should be reflected in the template.

What I saw

The first character typed in to the field was reflected on the template, any after that were not.

Software versions

Ember: v3.20.2 Ember Buffered Proxy: v2.0.0-beta.0

This as ok in Ember v3.17.0 which is what I am upgrading from.

Supporting info

I see that there was a fix here (46d00d71cc85726025e1c385892d59078640914c) and here (3f63beb49185f7c377104689deb6395d93d1b8b4) that "fixed" the same issue. The change was basically this:

- notifyPropertyChange(this, key);
+ notifyPropertyChange(content, key);

I have tested in my local code to reverse this change and it actually works.

So, maybe the bug that was originally "fixed" was actually working around a bug in Ember itself, that has since been patched, rendering the "fix" in this repo redundant.

I don't have enough context around how the notifyPropertyChange works and what changes have been made to Ember (and Glimmer?) to diagnose this myself.

I have created 2 codesandboxes (below) that reproduce this issue. The first is the working sandbox using Ember v3.17.0 and the second where the issue is present, using Ember 3.20.2.

Working: https://codesandbox.io/s/vibrant-browser-g4n7p Not working: https://codesandbox.io/s/happy-wilbur-btx3x

I'm happy to submit a PR that changes this code back, however, it feels like it might be worth actually understanding the reasoning as to why the original code stopped working in the first place and why it now works. I'd need some assistance in digging in to that.

/cc @tniezurawski @lukemelia

lukemelia commented 3 years ago

@achambers Sounds to me like we may need some version conditional behavior. notifyPropertyChange(this, key) seems to be more conceptually correct to me, so I would venture that the Ember behavior that necessitated the change was a bug.

achambers commented 3 years ago

@achambers Sounds to me like we may need some version conditional behavior. notifyPropertyChange(this, key) seems to be more conceptually correct to me, so I would venture that the Ember behavior that necessitated the change was a bug.

Yep, agreed

achambers commented 3 years ago

I’ve made the change on my fork. I’d need to dig in to understand exactly which versions use the content “fix” and then I can add some conditionals around that.

@lukemelia Any idea what the patterns are around for checking ember versions in code in order to conditionally run things?

lukemelia commented 3 years ago

@achambers check out https://github.com/pzuraq/ember-compatibility-helpers

achambers commented 3 years ago

@achambers check out https://github.com/pzuraq/ember-compatibility-helpers

Thanks dude. Will do. Will knock a PR together next week.

achambers commented 3 years ago

Ok, I've spent some time investigating versions (using this sandbox) and here's the list as I see it. I'll build a PR based on this:

Version	`notifyPropertyChange(this)`	`notifyPropertyChange(content)`
3.12	✅	❌
3.13	❌	✅
3.14	❌	✅
3.15	❌	✅
3.16	✅	✅
3.17	❌	✅
3.18	❌	✅
3.19	❌	✅
3.20	✅	❌
3.21	✅	❌
3.22	✅	❌
3.23	✅	❌
3.24	✅	❌

I started with 3.12 as this was the last version that this worked before https://github.com/yapplabs/ember-buffered-proxy/commit/46d00d71cc85726025e1c385892d59078640914c was introduced.
I have no idea what's up with v3.16. For some reason it works with both this and content. I can't explain this but I have verified it works 🤷‍♀️

So, based on the above table, I'll be using the following logic:

gte('3.13.0') && lt('3.20.0') -> use notifyPropertyChange(content)
anything else -> use notifyPropertyChange(this)

lukemelia commented 2 years ago

Fixed by #90

yapplabs / ember-buffered-proxy