Fixes for preboot and SSR error handling

[LotteHofstede]

Description

This PR fixes some issues with the preboot setting and the error handling fallback for SSR (Server-side rendering).

Instructions for Reviewers

Since the Angular CLI upgrade, the preboot setting in the environment files didn't work properly anymore.
Because ng serve is not yet properly supported by universal, the preboot option can't make a difference for the dev build. However, it is still sometimes useful to be able to run production mode without SSR, this is what we fixed in this PR.

The ejs template we added as a fallback for SSR in the Angular CLI PR, was meant to give some context about the request the server performed and pass it to the browser. Unfortunately, this never worked properly. Because this data is not being used anywhere yet, we decided against debugging the issue and simply removed the template.

The Zone.js error handler call in server.ts was removed, because we noticed that sometimes the response would still change after an error occurred, possibly causing the page to crash. The express error handler is a lot more robust in our experience.

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 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.

[lgtm-com]

This pull request introduces 1 alert when merging a6dd87f into 4b11e1d - view on LGTM.com

new alerts:

• 1 for Missing rate limiting

[artlowel]

This LGTM alert suggests adding a rate limiter to our express server. While this might be an idea worth considering, I believe it is out of scope for this PR.

[tdonohue]

@artlowel : Could you describe why you feel the LGTM suggestion is out of scope? From my reading of the LGTM notes, it's saying that this PR has added a file system access on this line: https://github.com/DSpace/dspace-angular/pull/863/files#diff-5c092da67283b0b787b618509fa7b96fR148 And LGTM is saying that can result in a DOS (denial-of-service) attack if a rate limit is not set on that file system access. Here's the LGTM rule & example: https://lgtm.com/rules/1506065727959/

That said, I did notice that sendFile call seems to only be triggered if an error occurs in SSR. But, maybe there's a way to set a rate limit just around that call?

[artlowel]

Because there was already a file system access, but since it was an ejs template LGTM apparently wasn't able to detect that.

@LotteHofstede has simplified it with this PR. Retrieving an html file from disk and returning it is surely less work than retrieving an ejs file, replacing any variables in it and then returning it.

[tdonohue]

@artlowel : Ah, ok. We can leave it as-is for now or tell LGTM to ignore it if we are confident a DOS cannot occur there.

[artlowel]

I'm not confident a DOS attack cannot occur here for the record. I'm only claiming that this PR doesn't make that any more likely than before.

But even so, before fixing it the way LGTM suggests we should perhaps discuss if a rate limiter is something we want to add. And if we do, whether we add it to express, or mention in the documentation as something to configure on httpd on nginx.

[tdonohue]

👍 @LotteHofstede and @artlowel : The code changes here look fine. I'm not seeing any changes in behavior based on main (which is a good thing).

That said, I've found it difficult to test that this is "working" as expected... I've run yarn start with preboot: true (default setting). I've tried triggering an exception by visiting an Item with a UUID that doesn't exist, but that doesn't seem to work fully on either main or in this PR. The result is that the UI just hangs with a "Loading..." message, even though I see a 404 returned and the commandline (where I ran yarn) shows an HttpErrorResponse message.

In any case, I think this is working as well as current main branch. I have no objections & the 404 issues seem unrelated to anything in this PR. Assuming you both agree, I'm OK with us merging this as-is, and doing further cleanup in a future PR as needed.

[artlowel]

@tdonohue The reason I suggested using a UUID that doesn't exist is because it is an error that is currently unhandled and should trigger the CSR fallback. So for the purpose of this test the expected behavior is that the UI is broken. To see whether the fallback has worked look for the message "Error in SSR, serving for direct CSR" in the server output.

I created a ticket to track the 404 issue: #865

[tdonohue]

@artlowel : Thanks for clarifying. I can verify that I do see "Error in SSR, serving for direct CSR." whenever I trigger an error by accessing an invalid UUID. So, this PR is working as expected!