-
Notifications
You must be signed in to change notification settings - Fork 11
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!: add support for Angular v17 #67
Conversation
✅ Deploy Preview for plugin-angular-universal-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
package.json
Outdated
@@ -23,18 +22,18 @@ | |||
"prepublishOnly:pull": "git pull", | |||
"prepublishOnly:install": "npm ci", | |||
"prepublishOnly:test": "npm test", | |||
"test": "jest" | |||
"test": "# no tests" |
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.
All the tests we had covered behaviour I had to remove, so i've removed the test script for now.
Finally fixed the Deploy! https://deploy-preview-67--plugin-angular-universal-demo.netlify.app/dashboard |
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.
Great that you've been able to simplify it so much! I have a few questions and comments
package.json
Outdated
"version": "1.0.1", | ||
"description": "Netlify Build plugin Angular Universal - Run Angular Universal seamlessly on Netlify.", | ||
"name": "@netlify/angular-runtime", | ||
"version": "2.0.0", |
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.
release-please should handle the versioning, though you may need to bootstrap it as a new package. I don't think it should be 2.0.0 though, as it's a new package so semver doesn't apply
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.
Let's stick with 2.0.0
so that the Git tags in this repo don't clash.
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.
If rlewase-please is setup ok then you shouldn't update this here: the breaking change will trigger a major bump anyway
I believe i've implemented everything necessary! Noice. Looking forward to release this :D |
const ssrFunction = ` | ||
import "./polyfill.mjs"; | ||
import { Buffer } from "node:buffer"; | ||
import { renderApplication } from "${toPosix(relative(edgeFunctionDir, serverDistRoot))}/render-utils.server.mjs"; |
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 think you can do path.posix.relative()
rather than implementing it yourself
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 can, but edgeFunctionDir
and serverDistRoot
will contain windows-style separators on windows. Not sure if path.posix.relative()
know how to deal with them
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.
Ah, good point
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.
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.
🎉
31b44b9 ensures that this also works out-of-the-box for |
"src/styles.css" | ||
], | ||
"scripts": [], | ||
"server": "src/main.server.ts", |
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.
manually verified that changing this doesn't change the output file name, so it's fine to depend on that in our generated edge function.
README.md
Outdated
To test this in local development, run your Angular project using `netlify serve`: | ||
|
||
```sh | ||
netlify serve --dir dist/demo/browser |
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 can remove --dir
here once netlify/cli#6133 lands
Added a way of working with |
@ascorbic could I bother you with one more review on the changes since you last looked? I've added a couple things, and am happy with the state of things now. |
Gonna move forward and merge this now, so I can fix any bugs that release-please might have. Feel free to still review, so we can address in a follow-up. |
This PR contains a big overhaul of our Angular Plugin, adding support for Angular v17. (I was sneaky and found the release candidates!)
As part of this, we're massively simplifying the plugin. We generate an edge function that thinly wraps around the output generated by
ng build
, there's no mangling withangular.json
required anymore.We're also renaming the package to
@netlify/angular-runtime
, to be more in-line with our other framework plugin names.