feat(schematics): firebase function deployment for angular universal #2305

mgechev · 2020-02-01T02:31:15Z

Checklist

Issue number for this PR: #2304
Docs included?: yes
Test units included?: yes
In a clean directory, yarn install, yarn test run successfully? yes

Description

This PR includes:

Refactoring of the ng-add schematics. It decomposes the function to two separate ones responsible for static file deployments and SSR. Unfortunately, I wasn't able to get rid of the extra schematic from collection.json since currently our APIs do not allow manually persisting the Tree on the disk.
Minor refactoring of the deploy builder to incorporate the functionality for server-side rendering enabled deployments.
Refactoring of the tests to reflect the updated structure of ng-add and the deploy action.
Implementation of deployment to Firebase functions.

This implementation supports Angular Universal version 9 and above. Originally I was thinking of checking the dependency versions manually with semver during ng deploy/ng add, but then decided that the peer dependency check that @angular/fire does might be sufficient.

Since the PR is relatively big, if you prefer I can decompose it into several smaller ones.

jamesdaniels · 2020-02-02T03:51:59Z

I'm fine with it being one big commit. Don't see any obvious red flags, I'll give it a proper review and a test drive come Monday. Will let you know how that goes. Thanks!

jamesdaniels · 2020-02-05T07:34:04Z

Simple case seems to be working well. Going through the code.

What do you think of making the deploy process for Universal like this by default though:

prerender
dist/browser gets deployed to Firebase hosting
dist/server gets deployed to Cloud Functions with the ** pass-through on hosting

That way all the static assets / prerenders are on the CDN w/o need for cold-start. We could also give the developer some options here:

? We detected an Angular Universal project. Would you like to prerender on deploy? Yes
? Deploy browser assets to Firebase Hosting? Yes
? Deploy server assets to Cloud Functions? Yes
UPDATE package.json (2302 bytes)
✔ Packages installed successfully.
✔ Preparing the list of your Firebase projects
? Please select a project: (Use arrow keys or type to search)
❯ Project A
  Project B
  Project C

mgechev · 2020-02-05T15:45:17Z

Currently, the generated firebase.json would look like this:

{
    "target": "project",
    "public": "dist/browser",
    "ignore": ["firebase.json", "**/.*", "**/node_modules/**"],
    "rewrites": [
      {
        "source": "**",
        "function": "ssr"
      }
    ]
  }
}

My understanding is that we deploy static assets to a CDN and we have the ** rewrite to a firebase function. I intentionally didn't do prerendering since I'm not sure if everyone would want to opt-into that.

jamesdaniels · 2020-02-05T22:26:16Z

Ok, fair enough on the prerender; we can always see if that's requested later down the road.

** will cache it behind the CDN, due to the maxAge in server.ts, but only after the first time it's requested and the file hasn't been deployed to hosting. It's a reverse proxy (on cache miss) model. Deploying the static assets to hosting first would ensure that they're the freshest, even if they didn't change the filename (different eviction model), and won't be subject to coldstart...

That said, I missed that you were already deploying the browser bundle to hosting. So ignore that feedback 😃

jamesdaniels · 2020-02-05T22:31:08Z

src/schematics/deploy/functions-templates.ts

+    "logs": "firebase functions:log"
+  },
+  "engines": {
+    "node": "8"


It's beta, but what do you think of defaulting to Node 10? I think it would be a better experience out of box.

Let's warn/error if the engine version doesn't match their local development.

jamesdaniels · 2020-02-05T22:32:11Z

src/schematics/deploy/functions-templates.ts

+    "node": "8"
+  },
+  "dependencies": {
+    "firebase-admin": "~7.0.0",


Can we make these versions configurable, maybe look at their dev deps?

Let's assume latest if they don't have in their own package.json

jamesdaniels · 2020-02-05T23:08:58Z

src/schematics/deploy/functions-templates.ts

+  "engines": {
+    "node": "8"
+  },
+  "dependencies": {


Is there a way we could allow configuration of webpack externals here? E.g, libs that they don't want bundled but use in SSR

jamesdaniels · 2020-02-05T23:10:45Z

Also general feedback, the deploy was less interactive than I would've liked. Can we pipe the output from the hosting / functions or put our own spinner / progress indicator?

jamesdaniels

LGTM, comments are nits. Feel free to address before I cut rc.1 or we can take as action items.

mgechev · 2020-02-11T18:19:40Z

Thanks for the review! I'll address some of the comments later this week. I'll probably not find the time for everything we discussed last week, but it shouldn't be a blocker for 6.0.0.

mgechev · 2020-02-12T00:58:48Z

@jamesdaniels I did a few improvements:

Node.js version check - basically, we verify if the local node version satisfies ^10.0.0
Added --preview flag
Now we're using the versions from package.json as you suggested

This PR includes: 1. Refactoring of the `ng-add` schematics. It decomposes the function to two separate ones responsible for static file deployments and SSR. Unfortunately, I wasn't able to get rid of the extra schematic from `collection.json` since currently our APIs do not allow manually persisting the `Tree` on the disk. 2. Minor refactoring of the `deploy` builder to incorporate the functionality for server-side rendering enabled deployments. 3. Refactoring of the tests to reflect the updated structure of `ng-add` and the deploy action. 4. Implementation of deployment to Firebase functions. This implementation supports Angular Universal version 9 and above. Originally I was thinking of checking the dependency versions manually with `semver` during `ng deploy`/`ng add`, but then decided that the peer dependency check that `@angular/fire` does might be sufficient. Since the PR is relatively big, if you prefer I can decompose it into several smaller ones.

michelepatrassi · 2020-10-11T09:02:34Z

@mgechev @jamesdaniels hey thanks for the great work!

I run ng add @angular/fire for an angular universal project (here's the result commit michelepatrassi/ng-starter@370526c) and I cannot grasp two changes:

added devDependencies like inquirer, open, firebase-admin...why my main project would ever need them? Firebase functions are in a subfolder usually, and all dependencies are installed there
the externalDependencies meaning for my server app with all the firebase stuff

What are these changes solving? Does the second one maybe solves something like the externals filter done here https://gist.github.com/michelepatrassi/242f1f0a867af99918977ea64787fcee#file-webpack-server-config-js ?

googlebot added the cla: yes label Feb 1, 2020

mgechev requested a review from jamesdaniels Feb 1, 2020

jamesdaniels reviewed Feb 5, 2020

View changes

jamesdaniels approved these changes Feb 5, 2020

View changes

jamesdaniels mentioned this pull request Feb 7, 2020

[Question] How to build angular 9 universal firebase function #2319

Closed

hiepxanh mentioned this pull request Feb 8, 2020

functions zone issue #2324

Closed

mgechev added 6 commits Jan 29, 2020

feat: add preview flag and check node version

Loading status checks…

178ac87

fix: update the value of the only option passed to firebase-tools

Loading status checks…

f0b565c

style: use spaces instead of tabs

Loading status checks…

cea6b31

test: fix broken test

Loading status checks…

a578506

feat: use firebase function deps from package.json if exist

Loading status checks…

f7d5655

mgechev force-pushed the mgechev:deploy-ssr branch from d3d42ab to ae8ba95 Feb 12, 2020

docs: add description of the preview flag

Loading status checks…

a8639fb

mgechev force-pushed the mgechev:deploy-ssr branch from ae8ba95 to a8639fb Feb 12, 2020

Merge branch 'master' into deploy-ssr

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

989bf18

jamesdaniels merged commit 882b254 into angular:master Mar 27, 2020
2 checks passed

2 checks passed

cla/google All necessary CLAs are signed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

peterennis mentioned this pull request Mar 27, 2020

feat(schematics): firebase function deployment for angular universal … peterennis/angularfire2#54

Merged

mgechev deleted the mgechev:deploy-ssr branch Mar 28, 2020

Dec	JAN	May
	27
2020	2021	2022

angular / angularfire

feat(schematics): firebase function deployment for angular universal #2305

feat(schematics): firebase function deployment for angular universal #2305

mgechev commented Feb 1, 2020

jamesdaniels commented Feb 2, 2020

jamesdaniels commented Feb 5, 2020 •

edited

mgechev commented Feb 5, 2020

jamesdaniels commented Feb 5, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jamesdaniels commented Feb 5, 2020

jamesdaniels left a comment

mgechev commented Feb 11, 2020

mgechev commented Feb 12, 2020

michelepatrassi commented Oct 11, 2020

angular / angularfire

feat(schematics): firebase function deployment for angular universal #2305

feat(schematics): firebase function deployment for angular universal #2305

Conversation

mgechev commented Feb 1, 2020

Checklist

Description

jamesdaniels commented Feb 2, 2020

jamesdaniels commented Feb 5, 2020 • edited

mgechev commented Feb 5, 2020

jamesdaniels commented Feb 5, 2020

This comment has been minimized.

jamesdaniels Feb 5, 2020 Member It's beta, but what do you think of defaulting to Node 10? I think it would be a better experience out of box. 👍 1

This comment has been minimized.

jamesdaniels Feb 5, 2020 Member Let's warn/error if the engine version doesn't match their local development.

This comment has been minimized.

jamesdaniels Feb 5, 2020 Member Can we make these versions configurable, maybe look at their dev deps?

This comment has been minimized.

jamesdaniels Feb 5, 2020 Member Let's assume latest if they don't have in their own package.json

This comment has been minimized.

jamesdaniels Feb 5, 2020 Member Is there a way we could allow configuration of webpack externals here? E.g, libs that they don't want bundled but use in SSR

jamesdaniels commented Feb 5, 2020

jamesdaniels left a comment

mgechev commented Feb 11, 2020

mgechev commented Feb 12, 2020

michelepatrassi commented Oct 11, 2020

jamesdaniels commented Feb 5, 2020 •

edited

jamesdaniels Feb 5, 2020
Member

It's beta, but what do you think of defaulting to Node 10? I think it would be a better experience out of box.

jamesdaniels Feb 5, 2020
Member

Let's warn/error if the engine version doesn't match their local development.

jamesdaniels Feb 5, 2020
Member

Can we make these versions configurable, maybe look at their dev deps?

jamesdaniels Feb 5, 2020
Member

Let's assume latest if they don't have in their own package.json

jamesdaniels Feb 5, 2020
Member

Is there a way we could allow configuration of webpack externals here? E.g, libs that they don't want bundled but use in SSR