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

Ensure CSR fallback only happens on actual errors #871

Merged
merged 1 commit into from Sep 22, 2020

Conversation

artlowel
Copy link
Member

References

Fixes issue introduced by #863

Description

This PR fixes an issue introduced by #863, which caused the error handler on the express server to be triggered when there were no errors.

Instructions for Reviewers

In #863 we switched to the native express error handler. But we noticed on dspace-demo that the error fallback happens for every request.

The reason is that it's not a dedicated error handler, but simply the next function in the chain that can make changes to the response. There's only an error if err is defined.

This PR fixes that issue by checking if there's an error, and if not, passing the data from the previous function (the angular app) through.

To test it, triggering a 404 (as suggested in #863) is not sufficient for causing an actual express error. So I suggest you simply add throw new Error('A test error'); to the constructor of ServerAppModule, in which case you should see Error in SSR, serving for direct CSR in the server output with a stack trace. And if you leave it out that fallback shouldn't trigger, so that message shouldn't be there.

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

@artlowel artlowel added the 1 APPROVAL pull request only requires a single approval to merge label Sep 21, 2020
@artlowel artlowel added this to the 7.0beta4 milestone Sep 21, 2020
@artlowel artlowel added this to Needs Reviewers Assigned in DSpace 7 Beta 4 via automation Sep 21, 2020
@artlowel artlowel self-assigned this Sep 21, 2020
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me. Thanks @artlowel !

DSpace 7 Beta 4 automation moved this from Needs Reviewers Assigned to Reviewer Approved Sep 22, 2020
@tdonohue tdonohue merged commit 6960597 into DSpace:main Sep 22, 2020
DSpace 7 Beta 4 automation moved this from Reviewer Approved to Done Sep 22, 2020
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Sep 29, 2023
[DSC-1079] Pagination on item edit page uses the same page number for each bundle

Approved-by: Andrea Barbasso
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants