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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Ember 3.20.4 breaks @sort #19101

Closed
st-h opened this issue Aug 20, 2020 · 9 comments
Closed

[Bug] Ember 3.20.4 breaks @sort #19101

st-h opened this issue Aug 20, 2020 · 9 comments
Labels

Comments

@st-h
Copy link
Contributor

st-h commented Aug 20, 2020

馃悶 Describe the Bug

I have a ember data one to many relationship:

@hasMany('project/invite', {async: false}) invited!: EmberArray<ProjectInviteModel>;

When I delete one of those ProjectInviteModel like:

await invite.destroyRecord();

and there is a dependent sort:

  invitationsSortKeys = ['email'];
  @sort('model.invited', 'invitationsSortKeys')
  sortedInvitations;

sortedInvitations is no longer updated when the record is deleted. It just keeps the old (now invalid) value. Exactly the same issue happens when using https://pzuraq.github.io/macro-decorators/

This has been working fine up to Ember 3.20.3.

馃敩 Minimal Reproduction

Describe steps to reproduce. If possible, please, share a link with a minimal reproduction.
likely related to #19097

馃實 Environment

  • Ember: 3.20.4
  • Node.js/npm: v14.7.0
  • OS: linux / macOS (not tested running ember on windows)
  • Browser: does not matter
@jonathannewman
Copy link

We ran into this too. We found that a work around is to replace the use of @sort with a computed that declares the proper dependencies and sorts the result.

Certainly not ideal, but by doing so, we were able to leverage Intl.Collator for comparison over the collection instead of the relatively slow localeCompare that is used internally for some small performance wins.

@rwjblue
Copy link
Member

rwjblue commented Aug 20, 2020

Thank you for reporting! If someone has the time, a failing test case would be really helpful to us (to help ensure we get it fixed, and prevent future regressions). Something like this test (which passes ATM 馃):

['@test removing from the dependent array updates the sorted array'](assert) {
assert.deepEqual(
obj.get('sortedItems').mapBy('fname'),
['Cersei', 'Jaime', 'Bran', 'Robb'],
'precond - array is initially sorted'
);
obj.get('items').popObject();
assert.deepEqual(
obj.get('sortedItems').mapBy('fname'),
['Cersei', 'Jaime', 'Robb'],
'Removing from the dependent array updates the sorted array'
);
}

@rwjblue rwjblue added the Bug label Aug 20, 2020
@jonathannewman
Copy link

jonathannewman commented Aug 20, 2020

I've been trying to reproduce it in the unit tests, but haven't been successful.
The issues we saw were all about templates not re-rendering when the content changed. Any suggestions about where to explore testing in that avenue?

@pzuraq
Copy link
Contributor

pzuraq commented Aug 28, 2020

This is likely because @sort relies on autotracking (previously it relied on observers), so it has the same issue templates had with not updating. #19111 should fix this as well.

@Kilowhisky
Copy link

I'm experiencing the same issue @jonathannewman described where updates are not propagated. In particular i'm seeing it related to running action=(action (mut model.edmodel.hasmany)) with ember 3.20.4 and 3.20.5. Backing back down to 3.20.3 resolves it.

This sounds related enough to not need a new issue, but i may be wrong here.

pix-service-auto-merge pushed a commit to 1024pix/pix that referenced this issue Sep 23, 2020
- fix problem with deprecated autotracking for @sort computed property (see emberjs/ember.js#19101
- replace relative environment config file import (ex: '../config/environment') by namespace import (ex: 'pix-certif/config/environment') in order to simplify future upgrade
- add ember-cli-update.json file in order to improve upgrade experience
- fix problem with Ember Mirage request object that now use PascalCase HTTP headers instead of greek-case request headers
- bump dependencies rspecting Ember CLI Update program
@snewcomer
Copy link
Contributor

@st-h Mind trying ember-data 3.22.0? I added tests and a fix that should match your behaviour here.

@st-h
Copy link
Contributor Author

st-h commented Oct 10, 2020

@snewcomer Thanks. Will give it a try. Just noticed though, that the offending @sort is now already migrated to native classes without the @sort:

  get sortedInvitations() {
    return this.args.invited?.sortBy('email', 'user').reverse();
  };

But I still have a few @sort annotations :)

@st-h
Copy link
Contributor Author

st-h commented Oct 19, 2020

I finally found some time to test the upgrade and so far I haven't noticed any issues. 馃憤

@sandstrom
Copy link
Contributor

I finally found some time to test the upgrade and so far I haven't noticed any issues. 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants