Skip to content
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

refactor(movies): migrate to use application builder SSR features #281

Merged
merged 16 commits into from
Mar 8, 2024

Conversation

eneajaho
Copy link
Collaborator

No description provided.

@vmasek
Copy link
Collaborator

vmasek commented Mar 6, 2024

pushed few things:

  • migrated functions to ESM so now they are compatible with Angular app esbuild
  • fixed the why functions were not deployed (only hosting)
    • they need to live in same json config with hosting
  • rxState migration went wrong 😄
    • in multiple cases there was problem of "used before declaration" that resulted in weird errors like .pipe is not a function or similar undefined related problems.
  • I also ditched the local run() of server app in server.ts as it resulted in express running on firebase importing the app(), this can be fixed if we needed by refactoring file structure, but I guess it is not a priority (I also rarely use it since firebase serve is much more reliable to debug than local run).

Copy link

github-actions bot commented Mar 6, 2024

Visit the preview URL for this PR (updated for commit b97d0a8):

https://angular-movies-a12d3--update-to-ng17-ssr-3n6ista4.web.app

(expires Sun, 07 Apr 2024 12:49:26 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f5e1819659ee450e0b7241d53391169f76c6f1d4

@vmasek
Copy link
Collaborator

vmasek commented Mar 6, 2024

What needs to be fixed:

@eneajaho
Copy link
Collaborator Author

eneajaho commented Mar 8, 2024

@vmasek I'm looking into icon ssr loader now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote it to the commit message, in short you can only specify one config json for deployment and if you split the hosting and functions into multiple configs, the hosting will never be associated with the functions rewrites thus SSR will not work correctly

Copy link
Member

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I check and we could do a bit of a cleanup. Will comment more...

@eneajaho eneajaho marked this pull request as ready for review March 8, 2024 12:38
@eneajaho eneajaho merged commit c998cac into update-to-ng17 Mar 8, 2024
1 of 3 checks passed
@eneajaho eneajaho deleted the update-to-ng17-ssr branch March 8, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants