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: add health variant & centralise assetVariant switch #1821

Merged
merged 8 commits into from
Apr 28, 2022

Conversation

orbitalsqwib
Copy link
Contributor

Problem

What problem are you trying to solve? What issue does this close?

In preparation for the new GoGovSG stack targeted at healthcare providers, a new health variant needs to be set up to serve the appropriate assets and color themes.

Additionally, the assetVariant environment variable controls which variant of the GoGovSG application is being built. However, it is repeatedly imported in multiple places, which may introduce unintended bugs or issues that are hard to track down due to the multiple points of failure.

Closes #1812

Solution

How did you solve the problem?

This PR duplicates assets from the gov and edu variants to create placeholder assets for the health variant, which should be replaced when the actual assets and design for the variant are finished. Moreover, it also centralizes the assetVariant env var import, which is then imported by other files, which ensures that there is only a single point where the environmental variable is read and processed for use in other files.

Features:

  • Adds the health variant to the codebase

Improvements:

  • Cleans up and centralizes the assetVariant environment variable switch
  • Removes the default gov variant that assetVariant defaults to when no value for the environment variable is provided
  • Migrates the webpack configuration file to use Typescript

Bug Fixes:

  • Updates the CI/CD actions and Jest tests to use the appropriate assetVariant value

Tests

  • Check that the variant is deployed to the correct url(s) (staging.for.sg / for.sg)
  • Check that the variant displays the correct assets
  • Check that the variant's features works as expected

Deploy Notes

Notes regarding deployment of the contained body of work. These should note any
new dependencies, new scripts, etc.

Note: This PR moves several dev dependencies to be normal dependencies as listed below:

  • webpack
  • @sentry/webpack-plugin
  • clean-webpack-plugin
  • html-webpack-plugin

New dev dependencies:

  • ts-node : required to use a typescript webpack configuration file

Copy link
Contributor

@gweiying gweiying left a comment

Choose a reason for hiding this comment

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

lgtm!

Thanks for centralising the switch, it is much cleaner than before. Also, great work on finding and ironing out the bugs in tests and Sentry with the default gov variant!

@yong-jie
Copy link
Member

yong-jie commented Apr 27, 2022

lgtm! Mind sharing the thought process behind moving webpack stuff (compile/transpile-time) away from dev-dependencies?

minor nitpicking that we can examine in the future:

  • ts-ignore in the webpack config file (have we exhausted alternative approaches to get those plugins to work without using ts-ignore?)

@orbitalsqwib
Copy link
Contributor Author

lgtm! Mind sharing the thought process behind moving webpack stuff (compile/transpile-time) away from dev-dependencies?

minor nitpicking that we can examine in the future:

* ts-ignore in the webpack config file (have we exhausted alternative approaches to get those plugins to work without using ts-ignore?)

Replying to this for future documentation:

After a short discussion, these concerns have been resolved, reasons below:

  • Moving the webpack plugins used in the typescript-based webpack configuration file was necessary as it would probably have to be transpiled once to commonjs before webpack could use the config, and therefore plugins in dev dependencies might not be "picked up" during the transpile process for webpack to use afterwards - though this could also be a false positive thrown by eslint. (more testing required in this case, but probably better to be safe than sorry)

  • Due to incorrect typings, the HTMLWebpackPlugin's chunkSortMode does not have a none option available, despite it being an option listed in their documentation. Additionally, the plugins array also restricts the types allowed in it to what was previously specified (e.g: plugins A, B and C are specified in an object first, and D is injected into the object's plugins array later, there will be a type error as the array is typed as accepting only plugins of type A | B | C). As this is incorrect behaviour, we have opted to use a ts-ignore to remedy the false positive.

@orbitalsqwib orbitalsqwib merged commit 01c4e54 into develop Apr 28, 2022
@orbitalsqwib orbitalsqwib deleted the feat/health-variant branch April 28, 2022 05:34
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.

Modify codebase to support 3 asset variants
3 participants