-
Notifications
You must be signed in to change notification settings - Fork 748
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
feat: update Angular template to version 17 #4280
feat: update Angular template to version 17 #4280
Conversation
🦋 Changeset detectedLatest commit: 9ec702b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
eb3fe50
to
6e598a4
Compare
console.log('render SSR', url.href); | ||
|
||
// Get the root `index.html` content. | ||
const indexUrl = new URL('/', url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to use an index file which is not part of assets? IE: not accessible from the browser. For context, this is because for SSR we have a dedicated page in server/index.server.html
which does not have critical css inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could inline the content of the file as an import here. Something like:
import document from "../server/index.html"
...
const content = await renderApplication(bootstrap, {
document,
url: url.pathname
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this and got it working for deployed apps and local app, but unfortunately we get a conflict in the allowed path name which means I couldn't get it working for both.
Since the index.server.html
is generated by Angular and only available post-build we have to convince TS and esbuild that this import is external. We can do this by using @ts-ignore
for TS and wrapping the import in a try-catch for esbuild.
let indexHTM: any;
try {
// @ts-ignore
indexHTML = await import("index.server.html");
} catch {}
const document = index.default
The deployed version requires the import to be "index.server.html"
but for wrangler pages dev
it must be "./index.server.html"
.
I couldn't find a good way to avoid this conflict.
6e598a4
to
805e29e
Compare
packages/create-cloudflare/src/frameworks/angular/templates/server.ts
Outdated
Show resolved
Hide resolved
805e29e
to
75f37e6
Compare
75f37e6
to
f63510b
Compare
`globalThis['process'] = {};`, | ||
`globalThis['global'] = globalThis;`, | ||
// Needed as performance.mark is not a function in worker. | ||
`performance.mark = () => {};`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be incorrectly polyfilled by wrangler. Which caused performance.mark
if not a function error.
Since this is not a Node API, do you reckon it can be fixed from your end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//cc @petebacondarwin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently mark()
is not supported:
https://developers.cloudflare.com/workers/runtime-apis/performance/
Looks like performance.now()
was added earlier this year - see cloudflare/workerd#603.
So perhaps a feature request for performance.mark()
might be accepted?
cc @jasnell
@alan-agius4 thanks a lot for the PR! 😃 Pete is off today, I feel like he'd be the best person to review this as he has by far the most context on it, he should be back tomorrow so I think we can wait for him 🙂 |
f63510b
to
212c6f8
Compare
Really excited about this one! Looking forward to move our Angular app for Mustakbil.com to Cloudflare. Tried twice in past, few months back and just yesterday without any success. I hope this pull request fixes the issues and will offer smooth experience. |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6719019888/npm-package-wrangler-4280 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6719019888/npm-package-wrangler-4280 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6719019888/npm-package-wrangler-4280 dev path/to/script.js Additional artifacts:npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6719019888/npm-package-cloudflare-pages-shared-4280 Note that these links will no longer work once the GitHub Actions artifact expires.
| Please ensure constraints are pinned, and |
Codecov Report
@@ Coverage Diff @@
## main #4280 +/- ##
==========================================
+ Coverage 75.34% 75.39% +0.04%
==========================================
Files 223 223
Lines 12341 12341
Branches 3190 3190
==========================================
+ Hits 9298 9304 +6
+ Misses 3043 3037 -6 |
I ran this via C3 locally and the Angular build fails... > npm run pages:build Results in these two errors:
|
212c6f8
to
cec1e67
Compare
cec1e67
to
87e5053
Compare
c6b4755
to
e805c06
Compare
This commits updates the Angular template to use Angular 17.
7996d67
to
7de8795
Compare
There is no point in running on other people's forks since they don't have access to the API token etc.
05ee48d
to
9ec702b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The e2e tests pass locally for me, they won't pass here due to the PR being on a contributor's fork.
We had to update the local pnpm version installed in the C3 package to fix the Angular e2e tests. Note to @jculvey - should we update that dependency automatically using dependabot?
@alan-agius4 this PR changes the behavior of the Cloudflare Angular template to SSR/Prerendering. Rationale: |
This commits updates the Angular template to use Angular 17.