Skip to content

feat: add basic support for angular universal #5

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 9 commits into from
Jul 26, 2021
Merged

Conversation

lindsaylevine
Copy link
Contributor

@lindsaylevine lindsaylevine commented Jul 12, 2021

This PR adds the minimum level support for an Angular Universal site like the site under /demo (aka the site from Jason's original proof of concept). Right now, it's missing some of the crucial monorepo logic that Matt built into the Next plugin. I think this can be excluded from a beta.

Callouts:

  • @nguniversal/express-engine is installed at the top-level for CLI usage with the demo build because the plugin is looking at the top-level. I guess this is part of the missing monorepo work. Can this be changed with minimal adjustments, am I missing something, or should we invest in adding getAngularRoot logic akin to getNextRoot in the next plugin?

To-Do in a separate PR:

  • ava test stuff
  • update repo standards per luke's notion doc
  • more graceful error handling for unmatched routes in builder func

@lindsaylevine lindsaylevine added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jul 12, 2021
@lindsaylevine lindsaylevine marked this pull request as ready for review July 21, 2021 10:11
@@ -47,7 +47,17 @@
},
"devDependencies": {
"@netlify/eslint-config-node": "^3.1.7",
"@nguniversal/express-engine": "^12.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a dependency, not dev dep?

Copy link
Contributor Author

@lindsaylevine lindsaylevine Jul 21, 2021

Choose a reason for hiding this comment

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

um, idk. i wasnt sure if the validateUniversal check only failed for the demo site on the CLI because of the unique setup

@lindsaylevine lindsaylevine changed the title feat: add basic support for angular universal alongside serverless schematic feat: add basic support for angular universal Jul 21, 2021
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.

Woohoo! This is awesome. I'd suggest waiting for @tzmanics to review it though.

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.

Great stuff. This is a lot neater. I wonder if there is any value in still allowing them to optionally pass in a projectName, in case they don't want to use the default. I don't really know enough about Angular to know if this is something that people might need.

@lindsaylevine
Copy link
Contributor Author

according to tara:

Screen Shot 2021-07-26 at 9 29 26 AM

we can always add later if needed

@tzmanics
Copy link
Contributor

This is really great! Nice work!

I notice that in the readme we say to create a netlify.toml that has different information from demo/netlify.toml. Should they be the same?

Copy link
Contributor

@tzmanics tzmanics left a comment

Choose a reason for hiding this comment

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

this is awesome! great work!!

@lindsaylevine
Copy link
Contributor Author

This is really great! Nice work!

I notice that in the readme we say to create a netlify.toml that has different information from demo/netlify.toml. Should they be the same?

@tzmanics what different information are you referring to? the build command is the same, just in an npm script. the extra configs ([[redirects]] and [functions]) will be removed when the CLI pieces get fixed by workflow. is there somethin else?

@lindsaylevine lindsaylevine merged commit 6967d54 into main Jul 26, 2021
@lindsaylevine lindsaylevine deleted the basic-support branch July 26, 2021 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants