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

Self contained front end and fixes for building on Heroku #829

Merged
merged 109 commits into from May 1, 2024

Conversation

jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Apr 26, 2024

What are the relevant tickets?

Load session from API and configure decoupled frontend for local development #616

Description (What does it do?)

Includes Self contained front end using Webpack to build HTML and Webpack Dev Server to serve #678 which failed to release due to the front end not being buildable on Heroku with the front end package and yarn workspace root having moved from the project root. The Heroku build has been tested and is successful.

The additional changes add the package.json back to the project root, though keep the frontends/package.json to retain a degree of self-contained frontend (Typescript, Jest configs and the watch container Dockerfile now live in ./frontends).

The watch container Dockerfile again needs the project root as the build context. There is a wider issue of yarn workspaces having the issue for monorepo setups that by hoisting node_modules and the lockfile to the workspace root, we don't have independently deployable workspaces that can be used as Docker context.

This also upgrades Yarn to 4.1.1 as I was having issues with the nesting resulting from the intermediary workspace in ./frontends. An in-workspaces package.json scripts excludes this and the root from being treated as workspaces for the previous yarn foreach commands.

How can this be tested?

Detail in #678

jonkafton and others added 30 commits February 21, 2024 16:12
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to have this in addition to the root level package.json? Having 2 levels of workspaces (root + frontends/workspace-name/` seems simpler than having 3 levels (root, + frontends/ + frontends/workspace-name/)

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 goal is for frontends to be self-contained. Unfortunately the issue of Heroku needing to find the package.json at root meant that it needed to be reintroduced there though, as we should only be needing to build the front end there temporarily, it seemed sensible to only pull what is needed to build rather than add all the front end test and lint config files etc back to the project root, so that we can more easily remove from the root once the front end is served elsewhere.

@jonkafton
Copy link
Contributor Author

This change fixes the react and react-dom versions to 18.2.0. Described in testing-library/react-testing-library#1310, there is an issue with 18.3.0 conflicting with @testing-library that breaks a lot of tests with error:

Warning: `ReactDOMTestUtils.act` is deprecated in favor of `React.act`. Import `act` from `react` instead of `react-dom/test-utils`. See https://react.dev/warnings/react-dom-test-utils for more info.

@jonkafton
Copy link
Contributor Author

This change fixes the react and react-dom versions to 18.2.0. Described in testing-library/react-testing-library#1310, there is an issue with 18.3.0 conflicting with @testing-library that breaks a lot of tests with error:

Warning: `ReactDOMTestUtils.act` is deprecated in favor of `React.act`. Import `act` from `react` instead of `react-dom/test-utils`. See https://react.dev/warnings/react-dom-test-utils for more info.

React released 18.3.1 last Friday to export act from React itself. Making that change when we upgrade should resolve the issue.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki 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 good except that docker compose up is broken. Missing command rebuild. Was it renamed?

I did test with running dev server outside of docker and all worked well.

docker-compose.yml Outdated Show resolved Hide resolved
Run the front end with:

```bash
yarn watch
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
yarn watch
yarn workspace frontends watch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added package scripts at root for these.

… moved to frontends so available inside container
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

Linting, tests, watch mode (backend + frontend) all seem good. Env vars are working.

👍

@jonkafton jonkafton merged commit 3bbf40b into main May 1, 2024
12 checks passed
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.

None yet

3 participants