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

Feat/#2148-client-Upgrade-to-Next-14 #2149

Merged
merged 11 commits into from
Jun 26, 2024
Merged

Conversation

kaaloo
Copy link
Contributor

@kaaloo kaaloo commented Jun 12, 2024

  • upgrade Next 14 related dependencies
  • added branch title setting for GH Pull Requests extension
  • fixed issue introduced in Move to yarn v1 and workspaces #2152 the following workflow to develop locally now works even if the server/dist directory is missing
yarn install
yarn dev:server
yarn dev:client # in another terminal for now

Fixes #2148

@matthieu-fesselier
Copy link
Collaborator

Works fine with me. Two things we could improve, but can be done later:

  • ⚠ The "images.domains" configuration is deprecated. Please use "images.remotePatterns" configuration instead.
  • Support for defaultProps will be removed: used in 2 of our old components, and a lot in reactstrap. It seems that reactstrap fixed some of the issues in version 9.2.0. We should try to upgrade it.

@kaaloo
Copy link
Contributor Author

kaaloo commented Jun 18, 2024

Works fine with me. Two things we could improve, but can be done later:

  • ⚠ The "images.domains" configuration is deprecated. Please use "images.remotePatterns" configuration instead.
  • Support for defaultProps will be removed: used in 2 of our old components, and a lot in reactstrap. It seems that reactstrap fixed some of the issues in version 9.2.0. We should try to upgrade it.

Ok yes, I think the images.domains deprecation should be part of this PR since it does concern the Next 14 upgrade. As for the defaultProps it could be worthwhile trying to fix as long as it's showing up on this branch. If the fix is too extensive then I'd definitely like to open a new issue and PR for this.

Copy link
Collaborator

@matthieu-fesselier matthieu-fesselier left a comment

Choose a reason for hiding this comment

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

except for the question about the server build script, perfect for me!

@@ -9,8 +9,8 @@
"main": "server.js",
"scripts": {
"prebuild": "tsoa spec-and-routes",
"build": "tsc",
"dev": "concurrently \"nodemon\" \"nodemon -x tsoa spec-and-routes\"",
"build": "yarn prebuild && tsc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it useful? It seems that prebuild is called automatically before the build. In this case, tsoa spec-and-routes is runned twice.
But for the dev script I think we should keep it, because the tsoa script is not complete before tsc runs, which causes an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes I noticed that with yarn v1, the prebuild script isn't called at all when running yarn build.. That's why I called it explicitly in the updated build script. I think the prebuild name is misleading though. On the other hand, when the dist directory is missing, running yarn dev:server was failing because of a race condition between the server and tsoa spec-and-routes. That was the reason for my update here.

@kaaloo kaaloo merged commit 91cab4c into dev Jun 26, 2024
1 check passed
@kaaloo kaaloo deleted the feat/#2148-client-Upgrade-to-Next-14 branch June 26, 2024 12:18
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.

[client] Upgrade to Next 14
2 participants