Skip to content

Conversation

@Duncan-Brain
Copy link
Collaborator

@Duncan-Brain Duncan-Brain commented Jun 11, 2025

Description

This PR copies all files from src that are not .cjs, .js, and .ts to the dist folder since non-javascript are not compiled as part of tsc.

This hopefully fixes the no-emails issue where the files are not found. Not 100% sure on this but maybe 90% sure this is it.

Notes:

  • rsync was added to dockerfile as an easy to understand utility, the find and copy was unreadable comparably and it seems like this is more standard for this now?
  • only added the copy script to dockerfile (not npm build) because the copy path would be different ./dist/app/src/ vs `./dist/api/src/ so it will work locally with a tweak to that path.

Jira link: LF-4842

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain):

Ran the docker container locally by adding this to my docker compose:

backend:
    container_name: litefarm-api
    deploy:
      restart_policy:
        condition: on-failure
        max_attempts: 3
    build:
      context: ./packages/
      dockerfile: ./api/prod.Dockerfile
    volumes:
      - /etc/letsencrypt:/usr/src/letsencrypt
    ports:
      - "5000:5000"
      - "5432:5432"
    environment:
      NODE_ENV: development
      DEV_DATABASE_HOST: host.docker.internal
      DEV_DATABASE: pg-litefarm
      DEV_DATABASE_USER: **insert user**
      DEV_DATABASE_PASSWORD: **insert password**

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have ordered translation keys alphabetically (optional: run pnpm i18n to help with this)
  • I have added the GNU General Public License to all new files

@Duncan-Brain Duncan-Brain marked this pull request as ready for review June 11, 2025 15:29
@Duncan-Brain Duncan-Brain requested review from a team as code owners June 11, 2025 15:29
@Duncan-Brain Duncan-Brain requested review from SayakaOno and navDhammu and removed request for a team June 11, 2025 15:29
Copy link
Collaborator

@navDhammu navDhammu left a comment

Choose a reason for hiding this comment

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

This looks like a good working solution - by copying the non-js, ts files over to dist. My main concern is that it may include every type of file and not just the ones we need, like .env or other hidden files that may somehow cause unintended side-effects.

Also, I noticed the dist output includes /shared and /.knex folders as well, so if there are any needed non-TS or JS files in those directories, they would also need to be copied.

I'm thinking its better to just copy the exact files we need? One alternate and more long-term solution that I saw someone mention somewhere, (I think it was in this discussion) is to create a dedicated static folder that will include all template files and other static assets needed by the backend in /api/static, and copy it over to /dist/api/static during build.

@kathyavini
Copy link
Collaborator

kathyavini commented Jun 12, 2025

Chatted a bit synchronously about this today, but updating here, too. I was looking at the files copied with --dry-run

rsync -avi --dry-run --exclude='*.js' --exclude='*.ts' --exclude='*.cjs' ./src/ ./dist/app/src/

and personally I like the idea of only copying templates/ as those are the only files we need -- it's a bit close to a static/ folder anyway for the API. Copying /shared also sounds good 👍 There aren't any files in there that are still needed, now that we've removed the old sensor flow, but I think we intend to start using that package more than we do. Sounds like a good idea to just set it up now, but optional for release since no relevant files right now.

@kathyavini kathyavini added bug Something isn't working enhancement New feature or request labels Jun 12, 2025
@Duncan-Brain
Copy link
Collaborator Author

Thanks @navDhammu good points.

I will update to restrict just to /templates folder and /shared/locales for now. The silent error on this one made me want to just add everything and avoid further issues since 'src` does not have those extra files -- but maybe not all are necessary at this time.

Thanks for the conversation about static. That might be a good idea! But not sure it would cover all cases or not.

Thanks @kathyavini for the options for dry-run that is pretty cool!

@navDhammu
Copy link
Collaborator

navDhammu commented Jun 13, 2025

@Duncan-Brain I noticed while reviewing that locally I'm not able run the backend server after building with tsc because its using dist/app as the entry point rather than dist/api, which I think was done to match the docker container's path. This felt a bit weird to have a script just so it works on docker, so I created a separate PR to address it. This might allow you to do the file copying in the build script rather than just in docker.

Something minor - maybe consider using copyfiles (or something similar) instead of rsync for better cross-platform compatibility. I believe rsync isn't supported on windows, so could help avoid issues for some contributors on windows.

@Duncan-Brain
Copy link
Collaborator Author

Duncan-Brain commented Jun 13, 2025

@navDhammu

I love the idea of aligning the folders will check that out! -- I mentioned this difference in my notes section of this PR and I hate that it is different haha

I like the compatibility idea! But I can't spend any more time on this now. copyfiles seems used by those others in the conversation so maybe it is the way to go! We have other scripts like "test-ci" and "scheduler" that also should not work on windows without WSL. An attempt was made on at least one of those scripts https://lite-farm.atlassian.net/browse/LF-2844 , https://lite-farm.atlassian.net/browse/LF-2929

@Duncan-Brain Duncan-Brain requested review from kathyavini and navDhammu and removed request for SayakaOno June 13, 2025 12:56
Fix templates syntax

Co-authored-by: Joyce Yuki <82857964+kathyavini@users.noreply.github.com>
@Duncan-Brain Duncan-Brain requested a review from kathyavini June 13, 2025 19:30
@kathyavini kathyavini added this pull request to the merge queue Jun 13, 2025
Merged via the queue into integration with commit fb4df72 Jun 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants