- Notifications
You must be signed in to change notification settings - Fork2.2k
chore(workspace): update sample app to Angular 20#3671
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
new file courtesy ofgrok-code-fast-1, we're doing something similar in other sample apps in the project, but if there's something here we don't need, let's call that out
| if(isMainModule(import.meta.url)){ | ||
| // eslint-disable-next-line @typescript-eslint/dot-notation | ||
| constport=process.env['PORT']||4000; |
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.
bothprocess.env.PORT (trying to access a property on an index type) andprocess.env['PORT'] (please use dot notation) have some sort of error associated with it, I went with this option
we may want to take a look at lint rules in the future
| @defer (hydrate on idle) { <app-database /> } @placeholder { Database! | ||
| … } @defer (hydrate on idle) { <app-firestore /> } @placeholder { | ||
| Firestore! … } @defer (hydrate on idle) { <app-functions /> } | ||
| @placeholder { Functions! … } @defer (hydrate on idle) { | ||
| <app-messaging /> } @placeholder { Messaging! … } @defer (hydrate on | ||
| idle) { <app-remote-config /> } @placeholder { Remote Config! … } | ||
| @defer (hydrate never) { <app-storage /> } @placeholder { Storage! … | ||
| } @defer (hydrate on idle) { <app-upboats /> } @placeholder { … } |
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'd recommend we start usingprettier for formatting (something to think about)
| "scripts": { | ||
| "ng":"ng", | ||
| "start":"firebase emulators:exec --import seed\"ng serve\"", | ||
| "start":"npx --yesfirebase-tools@latest emulators:exec --import seed\"ng serve\"", |
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.
maybe someone had firebase-tools installed globally, but I don't and I'd prefer to follow the pattern I see in the rootpackage.json of installing this on demand
alternatively, we could install the package in thedevDependencies and keep it up to date
@jamesdaniels any feedback for this PR? |
@jamesdaniels should we try to get this in before v21 comes along? 🤓 |
Uh oh!
There was an error while loading.Please reload this page.
Checklist
yarn install,yarn testrun successfully? yes, for thesampledirectoryDescription
Updating sample app to Angular 20
Author's Notes
test.tsfile -- it kept complaining about needing Zone.js even though this is a zoneless app, since there's only 3 tests, we may want to explore Vitest as an alternative--no-verifywhen committing this, there are ~350 lint errors in the repo and I didn't want to include those fixes in this change