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

upgrade typeorm to 0.3.x, clean up config and build #934

Merged
merged 9 commits into from
Jun 11, 2024

Conversation

sgfost
Copy link
Contributor

@sgfost sgfost commented Feb 2, 2024

API changes:

https://typeorm.io/changelog#breaking-changes-2

DataSource, connection/manager deprecation:

https://typeorm.io/changelog#deprecations


resolves virtualcommons/planning#65

@sgfost sgfost added server Server Side dependencies Pull requests that update a dependency file labels Feb 2, 2024
@sgfost sgfost marked this pull request as ready for review February 3, 2024 02:10
@sgfost
Copy link
Contributor Author

sgfost commented Feb 3, 2024

This came dangerously close to making me try out migrating to prisma. Typeorm gets to live another dayrelease

@sgfost sgfost changed the title upgrade typeorm to 0.3.20 upgrade typeorm to 0.3.x, clean up config and build Feb 23, 2024
sgfost and others added 6 commits June 5, 2024 20:14
https://typeorm.io/changelog#breaking-changes-2

this commit addresses find(), findOne(), and similar methods changes

TODO: migrate from manager/connection to DataSource
(https://typeorm.io/changelog#deprecations)

[no ci]
TODO: test setup is still broken

[no ci]
I think this functions more or less the same as the previous setup with
createConnection

* some minor clean up in terms of the setup routine
changed the server/.env generation from envsubst to sed and includes
remaining server secrets (DB_PASSWORD which is needed for datasource.ts
SECRET_KEY and MAIL_API_KEY which were previously read in from a file)

assumes that if .env exists it should not be overwritten. this does mean
managing additions to the versioned .env.template manually..
mirrors improvements in comses/comses.net#696

* use one shared .env for all non-secret config (replaces server/.env
  and client/.../config.ts)
* continue to use files for secrets, but with docker compose secrets
* move base_url mapping to shared/settings.ts from `configure` script
* replace server/deploy/ with top level deploy dir
* clean up anything unused or unecessary from Makefile
@alee
Copy link
Member

alee commented Jun 7, 2024

This came dangerously close to making me try out migrating to prisma. Typeorm gets to live another dayrelease

prisma looks interesting and a simpler way to map JS things to a database than typeorm which is one of those bring-familiar-abstractions-from-another-implementation (Hibernate / JPA / C# Entity Framework) to typescript

@alee
Copy link
Member

alee commented Jun 8, 2024

npm upgrade fails because of vitejs/vite#15714 - seems we'd need to upgrade to vite 5 (and vue 3?) to fix that. Trying to circumvent the previous issue by pinning @types/node to ~18.8 generates more whackamole compilation issues ala microsoft/TypeScript#51567

on staging RELEASE_VERSION doesn't seem to be getting set, maybe an issue with the client code / build having access to the shared Constants.RELEASE_VERSION and the environment?

Copy link
Member

@alee alee left a comment

Choose a reason for hiding this comment

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

LGTM though I think we should finish up the shared/src/settings.ts refactor and figure out why staging (and presumably production) isn't rendering RELEASE_VERSION properly

@@ -93,7 +93,7 @@ ccarra1@asu.edu
© 2020-{{ currentYear }}
<a href="https://www.azregents.edu/">Arizona Board of Regents</a> |

<a :href="constants.GITHUB_URL">{{ constants.BUILD_ID }}</a>
<a :href="constants.GITHUB_URL">{{ constants.RELEASE_VERSION }}</a>
Copy link
Member

Choose a reason for hiding this comment

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

for some reason this isn't being rendered properly on staging deployment, shows up as "unknown". Perhaps the shared/settings environment isn't being set at runtime?

should probably fully merge Constants into shared/src/settings as per its FIXME tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah I think it needs access to these at build time for them to be available. I'll try and do this with vite's support for env vars https://vitejs.dev/guide/env-and-mode

- use vite import.meta.env for things on the client that come from .env
  * would've been nice to just integrate this into shared/settings such
    that when called from the client, import.meta.env would be used
    instead of process.env but this is extremely difficult to do
- Constants -> settings
- add $settings vue global for accessing shared settings easier
vite needs the actual .env file in order to statically replace
import.meta.env values
@sgfost
Copy link
Contributor Author

sgfost commented Jun 11, 2024

shared settings and env vars should be fixed now. I haven't looked into the problems with upgrading yet

@alee
Copy link
Member

alee commented Jun 11, 2024

thanks Scott! LGTM, we can deal with dependency upgrades later

@alee alee merged commit 506416d into virtualcommons:main Jun 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file server Server Side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants