Cache redesign part 1

[artlowel]

References

• Fixes Cache redesign part 1: Don't remove expired objects #739
• Fixes Need alternative link index to make use of embeds #626
• Replaces Need alternative link index to make use of embeds #652

Description

This PR redesigns the cache as described in #739. It adds a "stale" state to remotedata rather than removing the objects involved when they exceed the duration they can be cached for. It also adds the ability to cache paginated lists.

It is no longer possible to retrieve a response directly from the request cache, everything has to work using RemoteData from now on, so a lot of code had to be rewritten to support that.

As a consequence most custom ResponseParsers were no longer necessary. The only cases where they've been kept is where rest returns objects that don't have both a self link and a (unique) type

This PR also adds support for alternative links (#626), as a consequence embeds should now can work almost everywhere (exceptions again are places where rest responses are out of the ordinary, such as facets and filters).

As the scope of this PR was already so big, I had to limit what I could fix in the scope. So I tried as much as possible not to touch anything above the dataservice level except in places where components relied on dataservices not using RemoteData

Instructions for Reviewers

The way to test this PR is, unfortunately, to try everything, as this PR affects anything that uses data from the rest api.

Keep an eye on the requests made in the network tab of your browser's dev tools to ensure that requests that should be cached aren't needlessly re-requested. In most cases I haven't touched custom cache durations for services, so pages like mydspace will still have their 10 second cache durations. Those will be revisted in part 2 of the cache redesign

In order to see what happens when objects become stale and do need to be re-requested, it's useful to reduce the default cache duration in your environment file. This will set it to 15 seconds:

  cache: {
    msToLive: {
      default: 15 * 1000
    }
  }

Before reporting an issue, please verify that it doesn't happen on https://dspace7-demo.atmire.com

Checklist

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes TSLint validation using yarn run lint
• My PR doesn't introduce circular dependencies
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[lgtm-com]

This pull request introduces 83 alerts and fixes 18 when merging 63c38e4 into aa8feb0 - view on LGTM.com

new alerts:

• 83 for Unused variable, import, function or class

fixed alerts:

• 17 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 7 alerts and fixes 24 when merging 6f4432d into aa8feb0 - view on LGTM.com

new alerts:

• 7 for Unused variable, import, function or class

fixed alerts:

• 23 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 24 alerts when merging 02280d5 into aa8feb0 - view on LGTM.com

fixed alerts:

• 23 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 24 alerts when merging b72d8a6 into aa8feb0 - view on LGTM.com

fixed alerts:

• 23 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 24 alerts when merging bea7fd3 into aa8feb0 - view on LGTM.com

fixed alerts:

• 23 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[tdonohue]

@artlowel : My apologies for the merge conflicts here. I just merged #955 and #925 ...and unfortunately most mergers may result in conflicts in this PR. If you could find time to clean these up, I'd appreciate it. In the meantime though, I'll start with a code review.

[tdonohue]

@artlowel : I've decided to break up my code review over several days...I've just finished reviewing the first 125 files & most of the code changes are small refactors & look good. Only hit upon two questions/comments (see inline below). I'll continue this review tomorrow (and likely into next week).

[lgtm-com]

This pull request introduces 1 alert and fixes 25 when merging afab886 into 32a29c4 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 24 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[tdonohue]

@artlowel : Stage 2 of my review is done & I've now (in total) reviewed the first 350 files. A couple more minor things noted inline below. I also highlighted a few classes which look to have missing specs (as adding those specs should increase the coverage so that there's no longer a red ❌ on this PR).

I also tested this PR today. Overall, it's looking great and it seems more speedy overall. I did not thoroughly test every feature, but I clicked around to most every page in the application & make some light edits to both existing objects and in-progress submissions.

The only issue I stumbled on (so far) is the following:

• The EPerson page only partially loads (http://localhost:4000/admin/access-control/epeople). This works fine on the demo site. Error seems to be TypeError: Cannot read property 'pageInfo' of undefined

[lgtm-com]

This pull request introduces 1 alert and fixes 25 when merging 5bf54ea into 32a29c4 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 24 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 25 alerts when merging 825847c into 32a29c4 - view on LGTM.com

fixed alerts:

• 24 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 25 alerts when merging eaba5a7 into 32a29c4 - view on LGTM.com

fixed alerts:

• 24 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 25 alerts when merging c725ce6 into 32a29c4 - view on LGTM.com

fixed alerts:

• 24 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 26 alerts when merging 7bc7096 into 32a29c4 - view on LGTM.com

fixed alerts:

• 25 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 26 alerts when merging cfaa9f4 into 32a29c4 - view on LGTM.com

fixed alerts:

• 25 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 26 alerts when merging 431deb1 into 39582c9 - view on LGTM.com

fixed alerts:

• 25 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[tdonohue]

👍 @artlowel : I've completed my code review (of all 515 files, whew!) and I have found no additional obvious code issues. You've already solved all my prior feedback. I also gave this another test today and I've found no issues so far. While it's difficult to test everything, I clicked around to every page (that I could think of), I tried creating a Community and Collection and submitting a new item, etc. I've found no issues.

Therefore, this looks good to me. If we were to find later issues we can clean them up as needed in separate tickets. Once @atarix83 has a chance to take a quick look or give this a quick test, I think this is ready to merge. Thanks @artlowel !

[lgtm-com]

This pull request fixes 33 alerts when merging c122b75 into 63e0c2f - view on LGTM.com

fixed alerts:

• 32 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 1 alert and fixes 33 when merging f5b4b6c into 17ca2c4 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 32 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 33 alerts when merging 4e18fa3 into 17ca2c4 - view on LGTM.com

fixed alerts:

• 32 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[atarix83]

thanks @artlowel for the great work

I've reviewed the code and did some general testing. Code looks good and all the issues specified seem to be resolved.

I only suggest to create an issue, if it doesn't already exist, to keep track that removeByHrefSubstring method is now deprecated and replace it with new method where it is still used

[artlowel]

Thanks for the review @atarix83 !

I only suggest to create an issue, if it doesn't already exist, to keep track that removeByHrefSubstring method is now deprecated and replace it with new method where it is still used

Yes, I've deprecated that method in the followup PR: #975, and further changes to it are planned in Part 3