[WIP BUGFIX] Prevent failed relationship fetches from causing infinite re-render and re-fetch #5492

runspired · 2018-06-15T01:08:00Z

Resolves #5479

Fix here is only for legacy build, I still need to port changes to the record-data build.
This will also need ported to 3.2 prior to release; however, doing so is likely non-trivial as a cherry-pick, so I will open a PR specific to 3.2 once this PR is complete.

cc @HeroicEric @fivetanley @rwjblue

rwjblue · 2018-06-19T23:23:17Z

How is this looking? I’d really like to get this landed and release 3.2..

runspired · 2018-06-19T23:47:19Z

@rwjblue need to add the fix you suggested for LTS, otherwise I think it's ready

runspired · 2018-06-20T06:13:23Z

.eslintrc.js

@@ -1,5 +1,6 @@
 module.exports = {
  root: true,
+  parser: 'babel-eslint',


I did not add this manually, unsure how this came in. @rwjblue any ideas?

Rebase issue maybe? I don’t think we should need a special parser should be able to remove...

igorT · 2018-06-20T11:45:37Z

addon/-legacy-private/system/relationships/state/belongs-to.js

+          return this._handleCompletedFind();
+        },
+        e => {
+          return this._handleCompletedFind(e);


splitting this into two methods might be much nicer

igorT · 2018-06-20T11:46:23Z

addon/-legacy-private/system/relationships/state/belongs-to.js

+    if (inverseInternalModel) {
+      if (shouldForceReload && inverseInternalModel.hasRecord) {
+        // reload record, if it is already loaded
+        promise = inverseInternalModel.getRecord().reload();


if we have a link, shouldn't we go fetch the link instead of reloading the record?

Added a code comment, but we branch prior to that.

igorT · 2018-06-20T11:48:21Z

addon/-legacy-private/system/relationships/state/has-many.js

+    return this.members.list;
+  }
+
+  get hasRelatedResources() {


Can you please write a comment describing the intended behavior of this method

This seems to be maybe equivalent to the hasLoaded property? If i'm reading this correctly, it should return true if you have an empty array, in which case the naming is pretty misleading

changed to allInverseRecordsAreLoaded

igorT · 2018-06-20T11:50:30Z

addon/-legacy-private/system/relationships/state/has-many.js

+
+  get hasRelatedResources() {
+    // check currentState for unloaded records
+    let hasEmptyRecords = this.currentState.reduce((hasEmptyModel, i) => {


Would this be cleaner to write as a find/filter?

unsure, I find I have to similarly pause and reason for any of the three approaches. Happy to embrace another pattern if there's one you prefer,

igorT · 2018-06-20T11:52:11Z

addon/-legacy-private/system/relationships/state/has-many.js

      }
    }

-    this.setRelationshipIsStale(true);
+    this.setHasFailedLoadAttempt(false);
+    this.setShouldForceReload(true);


Whats the thinking behind setting this on the object/interaction with the scheduleFetchMany?

unsure what you mean? this is for a forced reload..

igorT · 2018-06-20T11:53:04Z

addon/-legacy-private/system/relationships/state/has-many.js

    //TODO(Igor) sync server here, once our syncing is not stupid
    let manyArray = this.manyArray;

-    if (this.isAsync) {
+    if (this.shouldMakeRequest()) {


igorT · 2018-06-20T11:53:59Z

addon/-legacy-private/system/relationships/state/relationship.js

        => we get a new link for the relationship
     */
-    this.relationshipIsStale = !internalModel.isNew();
+    this.relationshipIsStale = !this.isNew;


Can you add a comment as to what the difference between forcing a reload and a stale relationship is

igorT · 2018-06-20T11:54:25Z

addon/-legacy-private/system/relationships/state/relationship.js

+      return false;
+    }
+
+    // Always make a request when forced


igorT · 2018-06-20T11:56:00Z

addon/-legacy-private/system/store.js

@@ -1871,7 +1871,7 @@ Store = Service.extend({
      snapshot: snapshot,
      resolver: resolver,
    });
-    emberRun.once(this, this.flushPendingSave);
+    emberRun.scheduleOnce('actions', this, this.flushPendingSave);


What's this about

Now that we have setup waiters for our backburner instance, we no longer need the autorun assertion. This allows folks (including ourselves) to use async await without wrapping everything in run loops.

igorT · 2018-06-20T11:56:25Z

tests/acceptance/relationships/has-many-test.js

@@ -0,0 +1,653 @@
+import { module } from 'qunit';


Many ❤️

skip record-data failures for now

DDOS is bad

…5499) * [BUGFIX beta] ports fix for infinite-relationship-retry from #5492 DDOS is bad * bump node version

runspired force-pushed the has-many-acceptance-tests branch from d172d4c to f4409bb Compare June 15, 2018 01:09

runspired commented Jun 20, 2018

View reviewed changes

igorT reviewed Jun 20, 2018

View reviewed changes

runspired force-pushed the has-many-acceptance-tests branch 2 times, most recently from 473abc4 to f311140 Compare June 20, 2018 20:39

adds tests and fix for infinite re-render

d725553

skip record-data failures for now

runspired force-pushed the has-many-acceptance-tests branch from b2a949b to d725553 Compare June 20, 2018 23:37

runspired added 2 commits June 20, 2018 17:01

drop explicit testing for 2.12

5d5b99b

add 2.18 LTS

7c01c60

runspired merged commit 4f53ebd into master Jun 21, 2018

runspired deleted the has-many-acceptance-tests branch June 21, 2018 04:46

runspired added a commit that referenced this pull request Jun 21, 2018

[BUGFIX beta] ports fix for infinite-relationship-retry from #5492

b5f34d8

runspired added a commit that referenced this pull request Jun 21, 2018

[BUGFIX beta] ports fix for infinite-relationship-retry from #5492

7021ff9

DDOS is bad

runspired added a commit that referenced this pull request Jun 21, 2018

[BUGFIX beta] ports fix for infinite-relationship-retry from #5492 (#…

3b7b5e7

…5499) * [BUGFIX beta] ports fix for infinite-relationship-retry from #5492 DDOS is bad * bump node version

runspired mentioned this pull request Aug 30, 2018

[META QUEST] Request for Community Help - RecordData! #5618

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP BUGFIX] Prevent failed relationship fetches from causing infinite re-render and re-fetch #5492

[WIP BUGFIX] Prevent failed relationship fetches from causing infinite re-render and re-fetch #5492

runspired commented Jun 15, 2018

rwjblue commented Jun 19, 2018

runspired commented Jun 19, 2018

runspired Jun 20, 2018

rwjblue Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

igorT Jun 20, 2018

runspired Jun 20, 2018

igorT Jun 20, 2018

[WIP BUGFIX] Prevent failed relationship fetches from causing infinite re-render and re-fetch #5492

[WIP BUGFIX] Prevent failed relationship fetches from causing infinite re-render and re-fetch #5492

Conversation

runspired commented Jun 15, 2018

rwjblue commented Jun 19, 2018

runspired commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment