Skip to content
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

Selected item not working #1147

Closed
DenimTornado opened this issue Aug 21, 2018 · 28 comments
Closed

Selected item not working #1147

DenimTornado opened this issue Aug 21, 2018 · 28 comments

Comments

@DenimTornado
Copy link

DenimTornado commented Aug 21, 2018

Template:
{{#power-select selected='Choose category' verticalPosition='below' options=categories onchange=(action "onchangeCat") as |category|}} <span>{{category.name}}</span> {{input value=category type="hidden" name="category"}} {{/power-select}}

I don't see that 'selected' element.

@broerse
Copy link

broerse commented Aug 28, 2018

Just tested with ember-data 3.4 but it seems all versions above 3.1.1 break ember-power-select. See also emberjs/data#5575

@runspired
Copy link

@broerse unsure what power-select is doing but I don't think the issue is with ember-data

@runspired
Copy link

@broerse
Copy link

broerse commented Aug 29, 2018

@runspired no above ember-data 3.1.1 https://github.com/cibernox/ember-power-select/blob/master/addon/templates/components/power-select/trigger.hbs#L1 does not detect change on the new proxy object.

@broerse
Copy link

broerse commented Aug 29, 2018

@runspired see the test on https://ember-twiddle.com/8699e6c887c9eb2cb4344fb1d2551dbe?openFiles=templates.application.hbs%2C if you roleback to ember-data 3.1.1 it works fine.

@broerse
Copy link

broerse commented Aug 29, 2018

@runspired you can also see this failing now on https://bloggr.exmer.com . Press Create and select an author.

@runspired
Copy link

@broerse so as I've said elsewhere, power-select should not rely on the proxy being swapped for a new instance.

It looks like from the twiddle that it works just fine in ember-data 3.4 for power-select, but the blockless version is broken. What's the implementation difference?

@jlami
Copy link
Contributor

jlami commented Aug 29, 2018

The problem actually lies here: https://github.com/cibernox/ember-power-select/blob/master/addon/components/power-select.js#L147

This is only called once if the blockless version is used due to the fact that an extra component level will prune the property change notification for the second component.
So the code does not triggers a second update to the internal state if the previous value is null.

@broerse so as I've said elsewhere, power-select should not rely on the proxy being swapped for a new instance.

@runspired But my feeling is that being able to listen to a belongsTo value should be possible. It has always been in the past. And not changing the Proxy only when it starts at null seems the breaking change here. Why not keep behaviour from the past, which I think is in line with normal ember behaviour?

@cibernox
Copy link
Owner

I just want to ACK on this. I'm not sure what changed in ember-data. There is certain parts of EPS that try to unwrap ED relations but not sure what might be going on.

If anyone can create a failing test that would be great of course, but I'll look into it either way.

@runspired
Copy link

Posting this comment here for visibility into what I suspect is going on: emberjs/data#5584 (comment)

@broerse
Copy link

broerse commented Sep 2, 2018

@cibernox I am not sure it can be fixed here because it seems to me that the ember-data change needs change on all levels. See Twiddle https://ember-twiddle.com/8699e6c887c9eb2cb4344fb1d2551dbe?openFiles=controllers.application.js%2C . There was a test created that nailed the problem but it was changed to test for other problem. See emberjs/data@e01c74f

@broerse
Copy link

broerse commented Sep 3, 2018

@cibernox @runspired Just until Ember is changed to work with the new ember-data above v3.1.1 we need the classic behavior to fix this issue. I released ember-data-classic v3.4.0 to work around this for now. You can see ember-power-select working again on https://bloggr.exmer.com/ Hope this will be resolved on all levels soon.

@Techn1x
Copy link

Techn1x commented Sep 4, 2018

Is there a workable workaround for this until ember-data releases a fix? Already running ember-data 3.2.1... don't particularly want to downgrade..

@runspired
Copy link

@broerse @Techn1x ember power select is at fault here, it needs to be changed to something less brittle. It’s possible that the behavior of ember’s component lifecycle hooks might also be improved; however, it makes no sense to do a buggy thing in ember-data to support a bug in power select.

@broerse
Copy link

broerse commented Sep 4, 2018

@Techn1x We switch all projects to ember-data-classic v3.4.0 and all components worked again. This is a temporary workaround for components that need to check for change 2 components deep. So it works for all components that need to do this not just ember-power-select.

@broerse
Copy link

broerse commented Sep 4, 2018

@runspired The change in ember-data is not necessary wrong but it break all components that need to check for change 2 components deep. ember-power-select works that way so perhaps it is better to say " ember is at fault here" ;-)

@runspired
Copy link

@broerse power-select can easily change to use a hook that does trigger (willRender), installs a computed property that watches the dependent keys appropriately, or watches content of PromiseProxies to recompute.

This can also be fixed by end users by resolving async relationships before passing them into the component.

@cibernox
Copy link
Owner

cibernox commented Sep 6, 2018

I updated the version of ember-data used in the addon and all the related tests are passing. Is there an easy way to create a reproduction I can use to find a workaround?

@broerse
Copy link

broerse commented Sep 6, 2018

@cibernox The failing test is here: martinic/ember-data-classic@d0cc97a

The temporary solution is here: martinic/ember-data-classic@c6b6fa4

And reproduction off the problem is here: https://ember-twiddle.com/8699e6c887c9eb2cb4344fb1d2551dbe?openFiles=controllers.application.js%2C

The problem is you can't see change 2 components deep. The ember-data guys say Ember must change to support this new behavior so if you can find a workaround until this is all resolved that would be great.

@runspired
Copy link

@broerse

  • that test doesn't test what it thinks it does
  • the temporary solution is not relevant here

The ember-data guys say Ember must change to support this new behavior

We don't say this. We say that Ember potentially could. However, this hook behavior has been well documented and known since 2.0.

The twiddle though is fairly good at showing the issue even if it is an overly complex replication (that attrs hooks only trigger for referential changes to attrs).

@broerse
Copy link

broerse commented Sep 7, 2018

@cibernox I don't agree with @runspired on the points above so please decide for yourself what the issue is.

I only changed the ember-data version to ember-data-classic to show the temporary solution solves the problem for now. See https://ember-twiddle.com/13467f39629dd7ce0448ba4601db2732?openFiles=twiddle.json%2C

Also take a look at emberjs/data#5575 (comment) by @jlami for more info on the problem.

@runspired
Copy link

@broerse as I’ve mentioned privately, please stop directing folks to your buggy fork of ember-data. It is not a solution, nor is it even a correct behavior for ember-data, it happens to work for a few apps but it likely breaks many more.

Furthermore, discouraging the proper fix from taking place here is equally disturbing.

It’s nice that you disagree that the test you pointed out is incorrect, but I’m sorry, that test is broken and not testing at all the behavior that you believe you are fixing.

Lastly, it’s nice that you “don’t agree” on what the ember and ember-data folks think about ember’s lifecycle hooks, but that doesn’t change the fact that their behavior in this regard has been known and documented since 2.0.

That a bug in ember-data allowed an incomplete implementation in power-select to work when it shouldn’t have for ember-data is a neat quirk of history, but ultimately power-select has this issue with any proxy or wrapper object today, not just ember-data relationships, and it is time to fix it.

@jlami
Copy link
Contributor

jlami commented Sep 7, 2018

Lastly, it’s nice that you “don’t agree” on what the ember and ember-data folks think about ember’s lifecycle hooks, but that doesn’t change the fact that their behavior in this regard has been known and documented since 2.0.

Could you point me to the documentation regarding this specific behavior?

@runspired
Copy link

One of many previous issues pointing this out emberjs/ember.js#12696 (it has also been a common pitfall folks have stumbled into when helping them in Slack).

Here's what the guides say which could be more clear.

didReceiveAttrs runs after init, and it also runs on subsequent re-renders, which is useful for logic that is the same on all renders. It does not run when the re-render has been initiated internally.

Since the didReceiveAttrs hook is called every time a component's attributes are updated whether on render or re-render, you can use the hook to effectively act as an observer, ensuring code is executed every time an attribute changes.

There are two things to note here:

  1. effectively, this is a re-render initiated internally (due to the bound property changing), although this is unclear and I wish the docs clearly stated that attribute changes are detected by reference.

  2. Note the language here: ensuring code is executed every time an attribute changes. This language is precise, but also not what most folks will think, and so again I think likely we should clarify it. Internal mutations to an object or an array do not mutate the reference. This is where Rob's comment in the above issue about computed properties still having a place in the glimmer component world applies.

@jlami
Copy link
Contributor

jlami commented Sep 7, 2018

@runspired Thanks for the reply. I agree that this language might be precise, but still not clear enough. It would certainly be good to elaborate on the distinction between an internal re-render and a normal one, and the role reference play here.
I think the problem is compounded here because the one-level deep component works fine. But only due to the notifyPropertyChange, which does not work between component levels. The fact that many components will work because of this makes it easy to write code that does not work when wrapped in another component.

I still think Proxy objects are not inherently constant and you could still (easily) choose to let the belongsTo computed just return a new instance.
You can say that constant Proxy references are correct code and should be allowed. But if it is a known problem that multi-level components wont work without extra (overly complicated) code, wouldn't it be easier to fix the problem at the source and just not use them? (But I'm curious if this could be solved cleanly on the user/power-select side of things)

@cibernox Would you like me to write a test that covers this?

jlami added a commit to jlami/ember-power-select that referenced this issue Sep 7, 2018
cibernox pushed a commit that referenced this issue Sep 7, 2018
@cibernox
Copy link
Owner

cibernox commented Sep 7, 2018

Can you all check if 2.0.6 fixes your problems with newer versions of Ember Data?

@broerse
Copy link

broerse commented Sep 9, 2018

@cibernox We tested it on some projects and we had to add checks for instanceof ObjectProxy but then all works again with ember-power-select v2.0.6 and the newest Ember Data. https://bloggr.exmer.com/ is also fixed with this version. Still think this check should not be necessary and goes against the Ember do what I want philosophy but that is no subject to discuss here. We are happy for now. Many thanks!

erikap added a commit to rollvolet/frontend-crm that referenced this issue Sep 16, 2018
Due to a change in ember-data the Proxy object of a belongsTo relation
doesn't always update anymore - unless set to a null value - but
proxy.content does. As a consequence, ember-power-select doesn't get
updated anymore on changes of the selected value if the proxy object is
passed as value. To fix this, we now pass proxy.content as value to ember-power-select. Note that
this makes the component unusable for non-proxy-object values.

Follow-up of the issue can be found in cibernox/ember-power-select#1147.
erikap added a commit to rollvolet/frontend-crm that referenced this issue Sep 16, 2018
Select component must have the same interface independent whether the
passed value is a plain object or an ObjectProxy with a content
attribute.
Due to a change in the behaviour of ObjectProxies in a belongsTo
relationship in Ember data the select components didn't get updated
correctly if an ObjectProxy is passed as value. This commit adds a
workaround so this behaviour is not exposed to the consumer of the
select components.

See also cibernox/ember-power-select#1147
kswilster added a commit to salsify/ember-power-select that referenced this issue Oct 1, 2018
* Resolves typo. (cibernox#1055)

* Added another addon: https://github.com/IliasDeros/ember-data-power-select

* v2.0.0-beta.1

* Fix but by bumping minimum version of ember-basic-dropdown

* selectedItemComponent into beforeOptionsComponent (cibernox#1060)

Make `selectedItemComponent` available in `beforeOptionsComponent`. This will allow for a persistent header showing the overall selected items inside of `power-select-multiple`. This matters in the case where the list ends up scrolling.

* v2.0.0-beta.2

* Fix sandboxing

* Add extra params for trigger (cibernox#1062)

* Add factoryFor in 2.10 ember-try scenario (cibernox#1063)

* Improve test-helpers import path (cibernox#1067)

* Improve test-helpers import path

* Still use ember-native-dom-helpers for find and findAll

* Document new import path

* v2.0.0-beta.3

* Fix bad code sample

* `horizontalPosition` property api reference update (cibernox#1071)

* Update how-to-use-it.hbs (cibernox#1080)

typo

* Update installation.hbs (cibernox#1078)

small typo

* Update installation.hbs (cibernox#1079)

this might be a typo as well

* Update action-handling.hbs (cibernox#1081)

Sorry to submit several PRs. I create them as I encounter typos.
I tried compile only one for action-handling.hbs this time tough ;-)

* stop click propagation added to the docs (cibernox#1087)

* stop click propagation added to the docs

* added more info about stop click propagation in the docs

* Update addon to 3.1 (cibernox#1088)

* Update addon to 3.1

* Smarter way to decide wether or not we want to load jquery

* Modernize test suite part 1 (cibernox#1089)

* Modernize test suite part 2 (cibernox#1090)

* Modernize test suite part 3 (cibernox#1091)

* Modernize test suite part 4 (cibernox#1092)

* Modernize test suite part 5 (cibernox#1093)

* Remove more usages of ember-native-dom-helpers

* Add more qunit-dom

* fixes cibernox#1094 (cibernox#1097)

* qunit-dom is almost everywhere (cibernox#1096)

* v2.0.0-beta.4

* Simplify tests a bit more

* Penultimate test refactor (cibernox#1098)

* Remove all traces of find/findAll from ember-native-dom-helpers

* Changed self.window to window and self.document to document (cibernox#1099)

* v2.0.0-beta.5

* Pre-render docs site at build time (cibernox#1102)

This PR enables Fastboot pre-rendering of the entire docs site using [prember[(https://github.com/ef4/prember). For URL discovery, it's using the newly-published [prember-crawler](https://github.com/ef4/prember-crawler)

I tested the `_redirects` on netlify to confirm it will work, and it does. By default, prember outputs an `_empty.html` file that contains your original (empty) index.html content, since your pre-rendered index.html file will have actual content in it.

* Making helpers more async (cibernox#1100)

* Fix last set of tests

* Add two more tests

* Improve component's behavior in beta (cibernox#1104)

* Fix typo

* Remove native dom helpers (cibernox#1105)

* Remove dependency on ember-native-dom-helpers

* Not used any more

* Update changelog

* v2.0.0

* Update ember-concurrency to fix bug in beta and canary (cibernox#1106)

* Drop node 4

* Add extra params for `selectedItemComponent` on trigger (power-select-multiple) (cibernox#1111)

* Fix: add extra hash to power-select-multiple selectedItemComponent

* Test: Add a couple missing tests for power-select-multiple with custom components

* Test: Update to use new `qunit-dom` test syntax

* v2.0.1

* Remove direct access to then (cibernox#1116)

* v2.0.2

* Remove console-log from test helper (cibernox#1119)

* typo (cibernox#1118)

* Remove console.log from test support (cibernox#1108)

* v2.0.3

* Bind title to the trigger (cibernox#1126)

* Fix tests failing due to new assertion in triggerKeyEvent (cibernox#1129)

* Replaced return value of maybePlaceholder for IE to undefined (cibernox#1128)

* v2.0.4

* Update to ember-cli 3.2 (cibernox#1132)

* Update to 3.3 (cibernox#1137)

* 2.X is not in beta

* guard against destroying hook when calling deactivate (cibernox#1144)

* 2.0.5

* Update to ember-cli 3.4 (cibernox#1149)

* Update to ember-cli 3.4

* Fix template-lint errors

* Update ember-data to 3.4 (cibernox#1150)

* Test and possible fix for cibernox#1147 (cibernox#1151)

* v2.0.6

* Only load polyfill when not in FastBoot environment (cibernox#1155)

fixes cibernox#1154

* v2.0.7

* v2.0.7

* fix: typeof returns string, broke polyfill when necessary (cibernox#1157)

* v2.0.8

* etc

* fixing tests

* circle node version

* migrate to circle 2
xomaczar added a commit to xomaczar/ember-paper that referenced this issue Oct 23, 2018
@Techn1x
Copy link

Techn1x commented Dec 5, 2018

FYI, we upgraded EPS and this fixed the issue were were having

kswilster added a commit to salsify/ember-power-select that referenced this issue Jan 3, 2019
* Resolves typo. (cibernox#1055)

* Added another addon: https://github.com/IliasDeros/ember-data-power-select

* v2.0.0-beta.1

* Fix but by bumping minimum version of ember-basic-dropdown

* selectedItemComponent into beforeOptionsComponent (cibernox#1060)

Make `selectedItemComponent` available in `beforeOptionsComponent`. This will allow for a persistent header showing the overall selected items inside of `power-select-multiple`. This matters in the case where the list ends up scrolling.

* v2.0.0-beta.2

* Fix sandboxing

* Add extra params for trigger (cibernox#1062)

* Add factoryFor in 2.10 ember-try scenario (cibernox#1063)

* Improve test-helpers import path (cibernox#1067)

* Improve test-helpers import path

* Still use ember-native-dom-helpers for find and findAll

* Document new import path

* v2.0.0-beta.3

* Fix bad code sample

* `horizontalPosition` property api reference update (cibernox#1071)

* Update how-to-use-it.hbs (cibernox#1080)

typo

* Update installation.hbs (cibernox#1078)

small typo

* Update installation.hbs (cibernox#1079)

this might be a typo as well

* Update action-handling.hbs (cibernox#1081)

Sorry to submit several PRs. I create them as I encounter typos.
I tried compile only one for action-handling.hbs this time tough ;-)

* stop click propagation added to the docs (cibernox#1087)

* stop click propagation added to the docs

* added more info about stop click propagation in the docs

* Update addon to 3.1 (cibernox#1088)

* Update addon to 3.1

* Smarter way to decide wether or not we want to load jquery

* Modernize test suite part 1 (cibernox#1089)

* Modernize test suite part 2 (cibernox#1090)

* Modernize test suite part 3 (cibernox#1091)

* Modernize test suite part 4 (cibernox#1092)

* Modernize test suite part 5 (cibernox#1093)

* Remove more usages of ember-native-dom-helpers

* Add more qunit-dom

* fixes cibernox#1094 (cibernox#1097)

* qunit-dom is almost everywhere (cibernox#1096)

* v2.0.0-beta.4

* Simplify tests a bit more

* Penultimate test refactor (cibernox#1098)

* Remove all traces of find/findAll from ember-native-dom-helpers

* Changed self.window to window and self.document to document (cibernox#1099)

* v2.0.0-beta.5

* Pre-render docs site at build time (cibernox#1102)

This PR enables Fastboot pre-rendering of the entire docs site using [prember[(https://github.com/ef4/prember). For URL discovery, it's using the newly-published [prember-crawler](https://github.com/ef4/prember-crawler)

I tested the `_redirects` on netlify to confirm it will work, and it does. By default, prember outputs an `_empty.html` file that contains your original (empty) index.html content, since your pre-rendered index.html file will have actual content in it.

* Making helpers more async (cibernox#1100)

* Fix last set of tests

* Add two more tests

* Improve component's behavior in beta (cibernox#1104)

* Fix typo

* Remove native dom helpers (cibernox#1105)

* Remove dependency on ember-native-dom-helpers

* Not used any more

* Update changelog

* v2.0.0

* Update ember-concurrency to fix bug in beta and canary (cibernox#1106)

* Drop node 4

* Add extra params for `selectedItemComponent` on trigger (power-select-multiple) (cibernox#1111)

* Fix: add extra hash to power-select-multiple selectedItemComponent

* Test: Add a couple missing tests for power-select-multiple with custom components

* Test: Update to use new `qunit-dom` test syntax

* v2.0.1

* Remove direct access to then (cibernox#1116)

* v2.0.2

* Remove console-log from test helper (cibernox#1119)

* typo (cibernox#1118)

* Remove console.log from test support (cibernox#1108)

* v2.0.3

* Bind title to the trigger (cibernox#1126)

* Fix tests failing due to new assertion in triggerKeyEvent (cibernox#1129)

* Replaced return value of maybePlaceholder for IE to undefined (cibernox#1128)

* v2.0.4

* Update to ember-cli 3.2 (cibernox#1132)

* Update to 3.3 (cibernox#1137)

* 2.X is not in beta

* guard against destroying hook when calling deactivate (cibernox#1144)

* 2.0.5

* Update to ember-cli 3.4 (cibernox#1149)

* Update to ember-cli 3.4

* Fix template-lint errors

* Update ember-data to 3.4 (cibernox#1150)

* Test and possible fix for cibernox#1147 (cibernox#1151)

* v2.0.6

* Only load polyfill when not in FastBoot environment (cibernox#1155)

fixes cibernox#1154

* v2.0.7

* v2.0.7

* fix: typeof returns string, broke polyfill when necessary (cibernox#1157)

* v2.0.8

* Fix typo in api-reference.hbs (cibernox#1158)

* use .set() to avoid assertion failure under Ember 3.4 (cibernox#1162)

* Add animationEnabled to be passed down to ember-basic-dropdown (cibernox#1165)

* Add highlightOnHoverEnabled to be used in options.js (cibernox#1166)

* Add highlightOnHoverEnabled to be used in options.js

When false, will disable automatically highlighting an option
when hovered over with the mouse. This will prevent unintentionally
selecting a new value when tabbing through a form with an EPS field
while the user's mouse is where the list will appear when the EPS
gains and then looses focus.

* Renamed highlightOnHoverEnabled to highlightOnHover

* v2.0.9

* Change slack for discord (cibernox#1169)

* close -> clone (cibernox#1176)

* close -> clone

'clone' is the intended word in the context, and the spelling error 'close' may cause some confusion to the reader

* spelling error fixes

close -> clone
debounding -> debouncing

just two small mistakes, can be confusing in context for the reader

* Fix build error (cibernox#1178)

* User active-voice instead of passive-voice

* Document using a computed property for the selected property

* Fix build error due to missing rel='noopener'

* Improve documentation (cibernox#1177)

* User active-voice instead of passive-voice

* Document using a computed property for the selected property

* Fix build error due to missing rel='noopener'

* minor typos

* Changed npm and bower for yarn

Changed instructions for contributing from `npm install` and `bower install` to `yarn install` as per cibernox#1179 to reflect updated build tools.

* Fix typo

* Allow select to open with click (cibernox#1181)

* v2.0.10

* v2.0.11

* Fix inconsistent focus of the input in single selects (cibernox#1180)

* v2.0.12

* v2.0.13

* v2.0.14

* v2.0.15

* Fix npm audit warnings (1 critical, 2 high) (cibernox#1183)

* Allow direct imports from node_modules (cibernox#1182)

* v2.1.0

* v2.2.0 (cibernox#1185)

* [BUGFIX] Allow test helpers to work with EBD >=1.0.5 (cibernox#1190)

In EBD 1.0.5 a fix for better a11y was introduced. It makes aria-owns to only be present on the trigger when
the dropdown is open. Test helpers in EPS relied in that property being there all the time, but now
they work both ways.

* v2.2.1

* Specify that only strings are searchable

* Update all dependencies December 2018 (cibernox#1194)

* Update all dependencies December 2018

* Fix sass complaint

* Fix tests

* Replace ember-cli-eslint with plain eslint (cibernox#1195)

* "You attempted to access the `length` property (of <(unknown):ember712>). (cibernox#1148)

Since Ember 3.1, this is usually fine as you no longer need to use `.get()`
to access computed properties. However, in this case, the object in question
is a special kind of Ember object (a proxy). Therefore, it is still necessary
to use `.get('length')` in this case.

If you encountered this error because of third-party code that you don't control,
there is more information at emberjs/ember.js#16148, and
you can help us improve this error message by telling us more about what happened in
this situation."
davelindquist-egistix added a commit to davelindquist-egistix/ember-power-select that referenced this issue Apr 16, 2020
Removed the check ```!(selected instanceof ObjectProxy)``` from the setter for ```selected```.  This check was added for issue cibernox#1147, but completely breaks powerselect's initial selection when dealing with an object that is 'async'.

The primary case here is when we pull an object out of a 'belongsTo' relationship, and use that as the 'selected' value to pass to powerselect; powerselect DOES have the code necessary to resolve the promise and deal with stuff, but because Ember Data passes it as an ObjectProxy, it completely skips this code!

Since we already have the check for "does it have a .then", why do we need the shortcircuiting on instanceof ObjectProxy?
@mkszepp mkszepp closed this as completed Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants