feat: support @19 #216

pieh · 2024-11-04T12:31:12Z

This adds support for Angular@19

It maintains same mechanism for Angular@17 and Angular@18 is it does currently, but for Angular@19 projects different setup is used.

The previous setup just no longer works (module we are relying on in build output is no longer produced), so we had to find alternative. With adjustments here we are inspecting user-space server.ts file and if it's a default one (search string for express in module content) we swap it out for one of our own that is compatible with Netlify edge function handler (+ some additional glue code that adds inline config etc).

With this change we are now using proper Public API so we should overall safe for any upcoming changes.

CommonEngine vs AppEngine:

CommonEngine is stable public API used to handle request handling - it works but it requires a bit of hacks (hijacking fs reads) but the biggest thing that it's stable API.
AppEngine is new alternative that is marked as Developer Preview (~experimental / unstable) - it's much easier to integrate with - it's also built without expecting node APIs (as ~runtime agnostic solution). It's also required to use AppEngine to make the most out of new Hybrid Rendering APIs that come with Angular 19 (also marked as "Developer Preview").
Because AppEngine is not yet marked as stable public API I opted to support both variants and it's up to customer to decide which one we will use - when scaffolding new Angular projects they are asked this question - after the fact they can still decide by adjusting their server.ts to use either one of those and we will pick this up and select appropriate config

netlify · 2024-11-04T12:31:25Z

✅ Deploy Preview for plugin-angular-universal-demo ready!

Name	Link
🔨 Latest commit	`76dae2b`
🔍 Latest deploy log	https://app.netlify.com/sites/plugin-angular-universal-demo/deploys/673b816981618500088c3c78
😎 Deploy Preview	https://deploy-preview-216--plugin-angular-universal-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…able CommonEngine

…veloper Preview App Engine

pieh · 2024-11-07T11:34:27Z

demo/src/app/app.routes.server.ts

+import { RenderMode, ServerRoute } from '@angular/ssr'
+
+export const serverRoutes: ServerRoute[] = [
+  { path: 'detail/:id', renderMode: RenderMode.Server },


Without providing this configuration Angular builds alone were failing with:

✘ [ERROR] The 'detail/:id' route uses prerendering and includes parameters, but 'getPrerenderParams' is missing. Please define 'getPrerenderParams' function for this route in your server routing configuration or specify a different 'renderMode'.

Note - this would be good opportunity to test Prerendering with fallback ( https://ng-dev-previews-fw--pr-angular-angular-58445-adev-prev-gwy7p64b.web.app/guide/hybrid-rendering#prerender-fallback-strategies), but it currently doesn't actually work:

⠴ Building... ✘ [ERROR] No matching export in "node_modules/@angular/ssr/fesm2022/ssr.mjs" for import "PrerenderFallback" src/app/app.routes.server.ts:2:21: 2 │ import { RenderMode, PrerenderFallback } from '@angular/ssr'; ╵ ~~~~~~~~~~~~~~~~~ ⠦ Building...

(TS types also suggest this named export should exist)

…er.ts" This reverts commit 8a7a7cb.

…r rendered

pieh · 2024-11-12T16:21:34Z

src/helpers/serverModuleHelpers.js

+const NetlifyServerTsAppEngine = /* typescript */ `import { AngularAppEngine, createRequestHandler } from '@angular/ssr'
+const angularAppEngine = new AngularAppEngine()
+
+// @ts-expect-error - createRequestHandler expects a function with single Request argument and doesn't allow context argument


Possibly this @ts-expect-error directive will be no longer needed IF angular/angular-cli#28845 would be accepted

Why would we need a @ts-except-error in a generated file in the first place? 🤔

Types are checked at ng build time and it would fail otherwise - see https://github.com/netlify/angular-runtime/actions/runs/11798216865/job/32863885795#step:6:1537 for example before I (re)added that @ts-expect-error directive.

There are options to change that ( angular/angular-cli#19948 ) but that's not the default.

There are some ways to maybe do things differently:

reqHandler named export is the one that would be used for dev server - and this one NEED to use createRequestHandler wrapped handler - given dev we wouldn't be getting context, so maybe this one could use use request

use other export for prod handler in which case we don't need to use createRequestHandler wrapping and no types to adhere to

Ok, I think I figured workaround - we can grab context from https://docs.netlify.com/edge-functions/api/#netlify-global-object and then we should be able to skip the type problem here by just not getting context from 2nd argument for edge functions and only grab request (1st param) which would be compatible with Angular's createRequestHandler without adjusting type like in my optimistic PR to @angular/ssr

I did end up making it work without those @ts-expect-error directives with usage of global Netlify object, but it still is not the best as context is typed as any because I can't use actual recent @netlify/edge-functions (that contain context on global object) because they reference Deno global types which Angular compiler doesn't know about those - users could install Deno types maybe to workaround this, but it does feel like some changes need to happen in @netlify/edge-functions to make it easy to use in Angular which doesn't typecheck in ~Deno runtime. Additionally to make it work I did add new module with getContext() export that basically just returns Netlify.context from global object, because directly trying to grab it from global Netlify was resulting in another type error in Angular land.

It generally seem like we either have to use any or we vendor NetlifyContext type (to not rely on @netlify/edge-functions package which is not usable in Angular context since @netlify/edge-functions@2.10.0 which added relience on global Deno types)

demo/server.ts

mrstork · 2024-11-14T15:31:23Z

src/index.js

@@ -29,8 +37,11 @@ module.exports = {
      IS_LOCAL: constants.IS_LOCAL,
      netlifyConfig,
    })
+
+    usedEngine = await fixServerTs({ angularVersion, siteRoot, failPlugin })


This function does two things:

replace server file

return the engine that's used

Ideally I think we split up these two ideas

We can also say that this function does half (step 1) of preparing edge handler and returned value is required to inform how second step should look like.

I did look at splitting this function but result is much more boilerplate and condition checks are needed as things that were handled by control flow in single function no longer get that and whatever state representation we return require adding much more conditions explicit that currently are just implicit due to control flow which I'm not sure is actually more readable than what currently is there, but maybe I miss some obvious model here that is better in all aspects?

mrstork · 2024-11-14T15:32:31Z

src/index.js

    })
  },
+  async onEnd() {
+    await revertServerTsFix()


Revert again? I see we do this in onBuild already. Could this cause file not found failures?

It flips boolean state to only revert once even if called multiple times - I just try to ensure this is executed as soon as possible (doing it onBuild) and having it in something like finally block to make sure it also executes on failures (onEnd). All of it in effort to not leave user code mutations hanging after builds and also trying to do it as early as possible (for the user interruption cases)

src/helpers/serverModuleHelpers.js

mrstork · 2024-11-14T15:38:11Z

src/helpers/serverModuleHelpers.js

+  const project = getProject(angularJson)
+  const {
+    architect: { build },
+  } = project


This block is the same as in other file, can potentially be extracted into a helper for getting build

I would slot this in follow up - won't touch this for now given time constraints with upcoming release so all the refactor-y type things I am skipping for now

mrstork · 2024-11-14T15:43:20Z

src/helpers/serverModuleHelpers.js

+ * @param {string} serverModuleContents
+ * @returns {'AppEngine' | 'CommonEngine' | undefined}
+ */
+const getUsedEngine = function (serverModuleContents) {


Anything we can hook into? This is brittle because user could have a TODO comment for example.
Perhaps something we work with the angular team on?

Yeah, this seems ok as a last-minute last-resort option, but it's beyond brittle and leaky and realistic scenarios that would break are easy to imagine (not purely hypothetical):

// TODO Switch to AppEngine when stable import { CommonEngine } from '@angular/ssr/node' /* ... */

With the things about known content ( #216 (comment) ) I think what seems most reasonable is to do this:

Compare against known default variants of server.ts file and if those match exactly - we can swap automatically as then we know there are no customizations there and our default is 1:1 replacement.

If server.ts is unknown - we can still try to find those keywords - but if both CommonEngine and AppEngine keywords are found - like in example above - the result of checking is "not sure". But we wouldn't use this information (even if just CommonEngine or AppEngine is found) and instead we only use this information to generate meaningful error message - if single engine is used - error message just show content their server.ts should have to use given engine - if no engines or both are found - we display both variants in error message (or we just display both variants always and just mention engine we discovered as most likely what "user wants" to not hide any information - just attempt to prioritize one we ~think is most relevant to user setup.

The part that is not clear to me however is how exactly I could check for wether current server.ts file is fine for our edge handler usage (aka wether user already use module in Netlify compatible way) - for now this is done by checking if express keyword is used which has pretty much all the same problems as selecting engine that is being used (easy to produce false positives)

mrstork · 2024-11-14T15:46:11Z

src/helpers/serverModuleHelpers.js

+  const usedEngine = getUsedEngine(serverModuleContents) ?? 'CommonEngine'
+
+  // if server module uses express - it means we can't use it and instead we need to provide our own
+  needSwapping = serverModuleContents.includes('express')


Once again this is quite brittle, maybe something we can hook into something.
This captures words that contain express as well.

What happens if the user customized this and added express on their own?

Maybe we can work with the framework team to improve this, perhaps we can even offer to open a PR for them

src/helpers/serverModuleHelpers.js

mrstork · 2024-11-14T16:19:11Z

src/helpers/serverModuleHelpers.js

+const { getProject } = require('./setUpEdgeFunction')
+
+// eslint-disable-next-line no-inline-comments
+const NetlifyServerTsCommonEngine = /* typescript */ `import { CommonEngine } from '@angular/ssr/node'


Obviously not in this PR, is anything blocking us moving to TS for our runtime code?

Obviously not in this PR, is anything blocking us moving to TS for our runtime code?

the /* typescript */ ~pragma is just something that treats content of following string literal in compatible editors that support it - for stuff like this - https://marketplace.visualstudio.com/items?itemName=bierner.comment-tagged-templates

It doesn't have any effect otherwise.

As for moving entire code of angular runtime to typescript - this is completely separate and I don't think there is blocker per se other than justifying this work being done - it would require migrating files and adding a build step/tools until stuff like https://nodejs.org/en/learn/typescript/run-natively is stable which I think is completely out of scope for this.

I'm not sure how much benefit we would see here because angular runtime itself is not overly complex and it uses jsdoc already - maybe in between solution would be to use // @ts-check to enable typechecking and ensure that jsdocs are indeed correct and everything is type safe ( https://www.typescriptlang.org/docs/handbook/intro-to-js-ts.html ), but still not in scope for this PR I would say

serhalp

Impressive work figuring all this out! 🤯

demo/server.ts

serhalp · 2024-11-15T21:48:55Z

src/helpers/serverModuleHelpers.js

+const NetlifyServerTsAppEngine = /* typescript */ `import { AngularAppEngine, createRequestHandler } from '@angular/ssr'
+const angularAppEngine = new AngularAppEngine()
+
+// @ts-expect-error - createRequestHandler expects a function with single Request argument and doesn't allow context argument


Why would we need a @ts-except-error in a generated file in the first place? 🤔

src/helpers/serverModuleHelpers.js

serhalp · 2024-11-15T22:06:25Z

src/helpers/serverModuleHelpers.js

+let needSwapping = false
+let serverModuleLocation
+let serverModuleBackupLocation


😬 hrm, module scope state? I guess since this is all running as part of a build plugin it can ever run twice without a process lifecycle, so it's probably ok. If it's easy I'd suggest refactoring it away though, since it's harder to reason about and introduces potential for future bugs and test nondeterminism.

Not super happy about this, but I just followed with the way we held state between build hooks already ( https://github.com/netlify/angular-runtime/blob/main/src/index.js#L8 ). I also don't really see good way to handle it that doesn't ultimately rely on holding state in module or global scope (that might just be abstracted away at first glance)

serhalp · 2024-11-15T22:12:26Z

src/helpers/serverModuleHelpers.js

+ * @param {string} serverModuleContents
+ * @returns {'AppEngine' | 'CommonEngine' | undefined}
+ */
+const getUsedEngine = function (serverModuleContents) {


Yeah, this seems ok as a last-minute last-resort option, but it's beyond brittle and leaky and realistic scenarios that would break are easy to imagine (not purely hypothetical):

// TODO Switch to AppEngine when stable import { CommonEngine } from '@angular/ssr/node' /* ... */

serhalp · 2024-11-15T22:15:43Z

src/helpers/serverModuleHelpers.js

+ *
+ * @returns {'AppEngine' | 'CommonEngine' | undefined}
+ */
+const fixServerTs = async function ({ angularVersion, siteRoot, failPlugin }) {


Quietly swapping out a user's file for another — potentially not running some of their custom code — doesn't seem ideal.

Is there any way we can differentiate between a fully untouched scaffolded default server.ts and a customized one, and only replace the former and throw an actionable error on the latter?

😬 I remember seeing that Vercel does this in their Remix adapter: https://github.com/vercel/remix/blob/d8eb9b02a8f803b11272fecbe037725750d0f510/packages/vercel-remix/vite.ts#L100-L109. They have to keep sha checksums of every template that's ever existed. 😰

... Can we chat with the Angular team about ways to make this and the engine check robust instead of relying on brittle hacks?

The general gist about silent swapping user code I do agree with, but this is essentially what we do already for Angular 17 and 18 in practice (we just not even use this file, despite users potentially applying customizations there) so this is not something we lose support for as we just never had it - just wanted to make sure this is known.

With above said - as we only care about this module for Angular 19 and later - we probably would be fine with just having ~hashes for the boilerplate that's generated with Angular 19 and treat older as unknown and force users to use one of ours then? Possible try generating current 17 and 18 boiler plate (which would only have CommonEngine) and hash them as CommonEngine ones.

More involved thing would be to look at https://github.com/angular/angular-cli/tree/main/packages/schematics/angular/ssr/files and server.ts.template files in those directories and try to hash those (after replacing vars with defaults?) with various git revisions.

The main question is just balance of what we supported historically and wether we want to go with:

fail build on unknown server.ts forcing user to adjust their code even if they have no actual customizations (maybe they have same code just reformatted due to adjusting prettier/editor settings in which case hashes wouldn't match with known ones)

don't fail build and try to be smart - this does lose potential customization but otherwise it ~"just work" and it's inline with current behavior for 17 and 18

... Can we chat with the Angular team about ways to make this and the engine check robust instead of relying on brittle hacks?

The difficult part here that the swap kind of has to happen before angular compilation - if we wouldn't want to swap automatically - then we could use some information from Angular Build (if it exist) to inform about build failure message (tailor the message to engine used in that module), but we wouldn't be able to use it to automate swapping (unless we hook into compilation itself)

mrstork · 2024-11-16T13:08:30Z

src/helpers/serverModuleHelpers.js

+    if (usedEngine === 'CommonEngine') {
+      await writeFile(serverModuleLocation, NetlifyServerTsCommonEngine)
+    } else if (usedEngine === 'AppEngine') {
+      await writeFile(serverModuleLocation, NetlifyServerTsAppEngine)
+    }


Perhaps just if/else because we've already forced it to be one or the other?
Are we protecting against future changes here?

This is just to make it super straight forward to follow logic without having to keep in mind that usedEngine at this point will be one of those 2 - there's not more to it

src/helpers/validateAngularVersion.js

…known variants

…th actionable error

pieh · 2024-11-18T17:47:26Z

src/helpers/serverModuleHelpers.js

+  const guessedUsedEngine = guessUsedEngine(serverModuleContents)
+
+  let errorMessage = `server.ts doesn't seem to be Netlify compatible and is not known default. Please replace it with Netlify compatible server.ts.`
+  if (guessedUsedEngine) {


this error case can be seen in https://app.netlify.com/sites/plugin-angular-universal-demo/deploys/673b7bee10573500083ea36f

…date demo site

…treated as lf

serhalp

💯

pieh added 8 commits November 6, 2024 13:20

chore: add angular@19 test fixture without prerendering and ssr

29bb85d

chore: add angular@19 test fixture with prerendering and ssr using st…

9916cfe

…able CommonEngine

chore: add angular@19 test fixture with prerendering and ssr using De…

2626142

…veloper Preview App Engine

chore: add scripts for installing new fixture dependencies

f70172e

test: add test for version validation when angular@19 is used

3ca0d57

test: add test for version validation when angular deps are missing

400f82b

chore: add entire .netlify dirs to .gitignore

3ac2776

test: add initial (failing) build tests for Angular 19 fixtures

210de56

pieh force-pushed the feat/support-v19 branch from 7fe10d0 to a50958a Compare November 7, 2024 08:29

pieh added 5 commits November 7, 2024 09:51

refactor: split reading angular version out to separate helper function

59ad25e

fix: handle prerendered-routes.json shape in Angular 19

314f3cb

chore: add runtime to new fixture dependencies

ae959d4

chore: upgrade demo to Angular 19

d1d3df3

fix: hopefully initial working demo deployment using CommonEngine

97b690b

pieh force-pushed the feat/support-v19 branch from a50958a to 97b690b Compare November 7, 2024 08:51

pieh commented Nov 7, 2024

View reviewed changes

pieh added 13 commits November 7, 2024 15:34

test: check if process import is result of using default server.ts

8a7a7cb

test: fix used fixture dir in one of tests

cf8a3d4

fix: only patch deno method in Deno env

394eff2

fix: try/catch deno.readFile patching to avoid potential fatal errors

83de2ed

chore: upgrade @netlify/build

5efa761

test: update assertion for changed hashes after update

ce65227

Revert "test: check if process import is result of using default serv…

b887b5f

…er.ts" This reverts commit 8a7a7cb.

fix: add engine name to 'not supported' error message

df8d0df

refactor: simplify user code

4cdf8a5

test: remove dashboard from expected excluded paths as it's now serve…

05460df

…r rendered

test: adjust test setup to not be impacted by angular-runtime devDeps

6187dfd

chore: adjust eslint directive after adding @angular/ssr dev dep

d6b104d

chore: upgrade node types (for before/after from node:test)

bd9994b

pieh force-pushed the feat/support-v19 branch from 1a658c7 to da1bc66 Compare November 12, 2024 15:30

pieh commented Nov 12, 2024

View reviewed changes

demo/server.ts Outdated Show resolved Hide resolved

pieh changed the title ~~WIP feat: support @19~~ feat: support @19 Nov 12, 2024

github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Nov 12, 2024

pieh marked this pull request as ready for review November 12, 2024 16:47

pieh requested a review from a team as a code owner November 12, 2024 16:47

pieh added 2 commits November 12, 2024 17:48

chore: remove outdated TODOs

945e676

chore: update jsdocs

deef2e4

mrstork reviewed Nov 14, 2024

View reviewed changes

serhalp reviewed Nov 15, 2024

View reviewed changes

mrstork reviewed Nov 16, 2024

View reviewed changes

src/helpers/validateAngularVersion.js Show resolved Hide resolved

pieh added 5 commits November 18, 2024 10:52

Merge remote-tracking branch 'origin/main' into feat/support-v19

d91b1d4

chore: upgrade to most recent RCs

57cd3f4

test: update assertion after package bump

66b9ccc

feat: initial boilerplate and tooling to start matching server.ts to …

e5036fd

…known variants

feat: only autoswap for known module content, otherwise fail build wi…

69dbcdf

…th actionable error

pieh force-pushed the feat/support-v19 branch from cbd6460 to 69dbcdf Compare November 18, 2024 17:39

pieh commented Nov 18, 2024

View reviewed changes

pieh added 3 commits November 18, 2024 18:49

fix: now that we require default or netlify compatible server.ts - up…

d1f0d1f

…date demo site

chore: drop unuseful comment

5d222f9

fix: attempt to normalize content for signature so crlf endlines are …

76dae2b

…treated as lf

mrstork approved these changes Nov 18, 2024

View reviewed changes

serhalp approved these changes Nov 18, 2024

View reviewed changes

pieh merged commit f6776f5 into main Nov 19, 2024
11 checks passed

pieh deleted the feat/support-v19 branch November 19, 2024 07:05

token-generator-app bot mentioned this pull request Nov 19, 2024

chore(main): release 2.2.0 #120

Merged

pieh mentioned this pull request Nov 19, 2024

fix: getContext() should be sync #222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support @19 #216

feat: support @19 #216

pieh commented Nov 4, 2024 •

edited

Loading

netlify bot commented Nov 4, 2024 •

edited

Loading

pieh Nov 7, 2024

pieh Nov 12, 2024

serhalp Nov 15, 2024

pieh Nov 18, 2024

pieh Nov 18, 2024

pieh Nov 18, 2024

mrstork Nov 14, 2024

pieh Nov 18, 2024

mrstork Nov 14, 2024

pieh Nov 18, 2024 •

edited

Loading

mrstork Nov 14, 2024

pieh Nov 18, 2024

mrstork Nov 14, 2024

serhalp Nov 15, 2024

pieh Nov 18, 2024

mrstork Nov 14, 2024 •

edited

Loading

mrstork Nov 14, 2024

pieh Nov 18, 2024 •

edited

Loading

serhalp left a comment

serhalp Nov 15, 2024

serhalp Nov 15, 2024

pieh Nov 18, 2024

serhalp Nov 15, 2024

serhalp Nov 15, 2024

pieh Nov 18, 2024 •

edited

Loading

mrstork Nov 16, 2024

pieh Nov 18, 2024

pieh Nov 18, 2024

serhalp left a comment

feat: support @19 #216

feat: support @19 #216

Conversation

pieh commented Nov 4, 2024 • edited Loading

netlify bot commented Nov 4, 2024 • edited Loading

✅ Deploy Preview for plugin-angular-universal-demo ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieh Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrstork Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieh Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

serhalp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pieh Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhalp left a comment

Choose a reason for hiding this comment

pieh commented Nov 4, 2024 •

edited

Loading

netlify bot commented Nov 4, 2024 •

edited

Loading

pieh Nov 18, 2024 •

edited

Loading

mrstork Nov 14, 2024 •

edited

Loading

pieh Nov 18, 2024 •

edited

Loading

pieh Nov 18, 2024 •

edited

Loading