Skip to content

fix: Catch router errors #21

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

Merged
merged 21 commits into from
Aug 5, 2021
Merged

fix: Catch router errors #21

merged 21 commits into from
Aug 5, 2021

Conversation

mraerino
Copy link
Contributor

@mraerino mraerino commented Aug 4, 2021

This adds a zone.js Zone to catch all errors bubbling up from rendering the app module.

In the process i made these refactors:

  • replace @nguniversal/express-engine with @nguniversal/common/engine for less indirection
  • code style fixes

@mraerino mraerino added the type: bug code to address defects in shipped code label Aug 4, 2021
@mraerino mraerino self-assigned this Aug 4, 2021
@netlify
Copy link

netlify bot commented Aug 4, 2021

✔️ Deploy Preview for plugin-angular-universal-demo ready!

🔨 Explore the source changes: 2c9dcde

🔍 Inspect the deploy log: https://app.netlify.com/sites/plugin-angular-universal-demo/deploys/610b100ebaff8e0007c231d8

😎 Browse the preview: https://deploy-preview-21--plugin-angular-universal-demo.netlify.app

} catch (e) {
const html = await render(event, context)
return {
// todo: how would people change the status code from inside angular code?
Copy link
Contributor Author

@mraerino mraerino Aug 4, 2021

Choose a reason for hiding this comment

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

@mraerino mraerino requested a review from lindsaylevine August 4, 2021 01:50
@mraerino mraerino marked this pull request as ready for review August 4, 2021 15:11
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Amazing work on this.Thank you! I'd be inclined to move the example stuff out of the demo for now though, and maybe put it in examples or something. We want demo to be as vanilla as possible (it's basically the standard Angular "Tour of Heroes" app)


export * from './src/main.server'
const html: string = await new Promise((resolve, reject) => {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding /// <reference types="zone.js" /> to the top of the file should make this work

const getAngularBuilder = ({ functionServerPath }) => `
const outdent = require('outdent')
// this is here so this works: https://marketplace.visualstudio.com/items?itemName=zjcompt.es6-string-javascript
const javascript = outdent
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice trick! I wonder if we should update our eslint rules so we could use an inline comment instead though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i initially just added outdent because i didn't like the leading spaces in the rendered files. then just aliased it to this name for convience

if (!!query) {
query = '?' + query;
}
return "https://" + event.headers.host + event.path + query;
Copy link
Contributor

Choose a reason for hiding this comment

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

event.headers['x-forwarded-proto'] || 'http'

@mraerino mraerino requested a review from ascorbic August 4, 2021 22:05
@mraerino
Copy link
Contributor Author

mraerino commented Aug 4, 2021

i reverted my changes to the demo site and addressed the comments.

Copy link
Contributor

@lindsaylevine lindsaylevine left a comment

Choose a reason for hiding this comment

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

marcus, you're an absolute legend for this. we owe you x10000!!!! 🙏 🙇

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Awesome!

@lindsaylevine lindsaylevine merged commit 066d97d into main Aug 5, 2021
@lindsaylevine lindsaylevine deleted the fix/catch-404 branch August 5, 2021 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants