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

chore: replace local module with shared folder #1144

Merged
merged 15 commits into from May 27, 2021
Merged

Conversation

lamkeewei
Copy link
Contributor

@lamkeewei lamkeewei commented Apr 20, 2021

Problem

Closes #1139

Solution

Use Typescript project references to reference shared module

I've chosen to take a slightly different approach to referencing the shared module in tsconfig as compared to the vaccinegovsg-clinic repository. Instead of adding the shared folder path in the include option, I've chosen to make use of project references instead.

The main advantage for this approach is that we can avoid having to manually keep in sync the dependencies for the shared folder and the other modules. For example, if we were to use the include approach, the xss library has to be in each of the package.json for backend, frontend and worker. Keeping all the different package.json in sync is a non-trivial problem and is prone to developer error.

The drawback of this approach is that we have to manually npm install inside the shared folder to install its dependencies. The necessary changes to .travis.yml and the respective Dockerfile have been made.

Manually patch moduleNameMapper rather than use aliasJest

The default aliasJest function provided by react-app-rewire-alias does not work for our setup. The main reason for this is that the library assumes that our aliases would follow the pattern of module/submodule. However, there are quite a number of instances in our code that references aliases directly without a submodule (e.g. import config from 'config').

Tests

Apart from changing the structure of the code, this PR should not modify or introduce any behaviour. To verify that everything works as expected:

  1. Ensure all tests for backend and frontend passes
  2. Ensure deployment succeeds without errors for backend and frontend

In addition, hot reload should also work now for the shared module. Changing code in the shared module should automatically trigger a recompilation for any other module depending on it.

* develop:
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
  feat: add unit tests for error states in critical workflows (#1118)
  feat: support whitelisting domains through `agencies` table (#1141)
  feat: add tests for happy paths in critical workflows (#1110)
  fix: prevent campaign names from causing dashboard rows to overflow (#1147)
  fix(email): Fix SendGrid fallback integration (#1026)
Removed npm test:watch as we can just use npm t -- --watch instead.
Did this to avoid duplicating pretest step.
@lamkeewei lamkeewei marked this pull request as ready for review May 7, 2021 03:34
script:
- npm run build
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help explain why we didn't need the build step previously but have included it now? I guess its to build the shared module, but where was the backend being built previously

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - this is to build the shared module. Previously there was not need to separate build the backend because npm test handles it. However, ts-jest does not automatically compile project references.

Copy link
Contributor

@miazima miazima left a comment

Choose a reason for hiding this comment

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

Tested the following and lgtm

  • All tests for backend and frontend passes
  • Deploys successfully on backend and frontend
  • Hot reload works for the shared module

Need to resolve merge conflicts

* develop:
  chore: upgrade dependencies (#1180)
  Backend tests - updating template for channels (#1175)
  feat(rich-text): support image as links (#1158)
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
@lamkeewei lamkeewei merged commit 06439df into develop May 27, 2021
@lamkeewei lamkeewei deleted the shared-folder branch May 27, 2021 08:05
lamkeewei added a commit that referenced this pull request May 27, 2021
* develop:
  chore: replace local module with shared folder (#1144)
  1.24.0
  Backend tests for campaign middleware and sms campaign routes (#1146)
  chore: upgrade dependencies (#1180)
lamkeewei added a commit that referenced this pull request May 27, 2021
* develop:
  chore: replace local module with shared folder (#1144)
  1.24.0
lamkeewei added a commit that referenced this pull request May 28, 2021
* develop:
  chore: replace local module with shared folder (#1144)
  1.24.0
  Backend tests for campaign middleware and sms campaign routes (#1146)
  chore: upgrade dependencies (#1180)
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.

Replace local modules with shared folder
2 participants