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

fix(deploy): handle server file #10061

Merged
merged 4 commits into from
Feb 29, 2024
Merged

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Feb 23, 2024

At first this PR was similar to #10055 but for Flightcontrol. But in the process of testing that and updating deploy target CI, I noticed a few other things were off:

  • coherence logic for the server file was flipped. fixed here
  • in general, a few duplicate ways of checking for the server file. tried to dedupe them the best i could without making massive changes
  • stylistic change to render
  • the flightcontrol setup wasn't handing corepack

Test as much as I could locally. Going to get this one into next so I can test in deploy target CI.

@jtoar jtoar added the release:fix This PR is a fix label Feb 23, 2024
@jtoar jtoar added this to the next-release-patch milestone Feb 23, 2024
@jtoar jtoar force-pushed the ds-flightcontrol/handle-server-file branch from 6fbfed3 to cff7745 Compare February 29, 2024 01:27
@jtoar jtoar force-pushed the ds-flightcontrol/handle-server-file branch from cff7745 to e2eac7f Compare February 29, 2024 06:30
@jtoar jtoar changed the title fix(flightcontrol): handle server file fix(deploy): handle server file Feb 29, 2024
@@ -38,7 +38,7 @@ services:
plan: free
env: node
region: oregon
buildCommand: corepack enable && yarn && yarn rw build api
buildCommand: corepack enable && yarn install && yarn rw build api
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just stylistic to be consistent with the web build command above

Copy link
Contributor Author

@jtoar jtoar Feb 29, 2024

Choose a reason for hiding this comment

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

Tested locally

const apiProdCommand = ['yarn', 'rw', 'build', 'api', '&&']
if (!hasServerFile) {
if (serverFileExists()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic was flipped here and was a bug

Copy link
Contributor Author

@jtoar jtoar Feb 29, 2024

Choose a reason for hiding this comment

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

Tested locally

export const isServerFileSetup = () => {
if (!serverFileExists) {
if (!serverFileExists()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to have been a bug here where we were just checking the truthiness of a function

Comment on lines +83 to +84
const serverFilePath = path.join(rwjsPaths.api.dist, 'server.js')
const hasServerFile = fs.pathExistsSync(serverFilePath)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still checking dist specifically here because this command is called after build

Comment on lines -61 to -62
extendEnv: true,
cleanup: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These default to true

@jtoar jtoar marked this pull request as ready for review February 29, 2024 06:48
@jtoar jtoar merged commit 76e27ed into main Feb 29, 2024
41 checks passed
@jtoar jtoar deleted the ds-flightcontrol/handle-server-file branch February 29, 2024 07:13
jtoar added a commit that referenced this pull request Feb 29, 2024
At first this PR was similar to
#10055 but for Flightcontrol.
But in the process of testing that and updating deploy target CI, I
noticed a few other things were off:

- coherence logic for the server file was flipped. fixed here
- in general, a few duplicate ways of checking for the server file.
tried to dedupe them the best i could without making massive changes
- stylistic change to render
- the flightcontrol setup wasn't handing corepack

Test as much as I could locally. Going to get this one into next so I
can test in deploy target CI.
@jtoar jtoar modified the milestones: next-release-patch, v7.0.5 Feb 29, 2024
jtoar added a commit that referenced this pull request Feb 29, 2024
At first this PR was similar to
#10055 but for Flightcontrol.
But in the process of testing that and updating deploy target CI, I
noticed a few other things were off:

- coherence logic for the server file was flipped. fixed here
- in general, a few duplicate ways of checking for the server file.
tried to dedupe them the best i could without making massive changes
- stylistic change to render
- the flightcontrol setup wasn't handing corepack

Test as much as I could locally. Going to get this one into next so I
can test in deploy target CI.
dac09 added a commit to dac09/redwood that referenced this pull request Feb 29, 2024
* 'main' of github.com:redwoodjs/redwood:
  RSC: Fix server build root (redwoodjs#10076)
  chore(release): update changelog
  RSC: Add build step debug logs (redwoodjs#10078)
  fix(deploy): handle server file (redwoodjs#10061)
  RSC: Fix node-loader message typo (redwoodjs#10077)
  chore(release): update changelog
  chore(docs): Add link to SuperTokens auth (redwoodjs#10067)
  chore(release): update changelog
  RSC: chore - upgrade @tobbe.dev/rsc-test to v0.0.5 (redwoodjs#10073)
  chore(.vscode): Enable find inside root __fixtures__ (redwoodjs#10072)
  chore(deps): bump es5-ext from 0.10.62 to 0.10.64 (redwoodjs#10071)
  fix(coherence): update setup command to detect server file (redwoodjs#10060)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant