Feat: Upgrade major deps and to Webpack 5

[harishv7]

Problem

This is a major PR that has some breaking changes. Currently, most of our dependencies are still using old versions. We are using outdated versions of key dependencies like Chakra UI, React, Design System and dev dependencies like Cypress.

Solution

This PR addresses the issue (#1196) by upgrading the dependencies. Along with this we will be upgrading to Webpack 5 which is required by the dependencies. Some of the key changes:

• Chakra from 1.x to 2.5.1
• React from 16 to 18 (required by Chakra 2 as well)
• Design system to 0.0.12
• React-scripts from version 4 to 5
• Update sentry to 7.x
• Update storybook to 6.5.x
• Remove craco from storybook config
• Update of CI workflow files to use latest packages/yarn
  Node version used for testing & dev: 18.15.0

Breaking Changes

• Yes - this PR contains breaking changes
  • Details ...
• No - this PR is backwards compatible

Features

Following on from the older PR,

Chakra Updates

• updating to chakra 2.x removes the isFullWidth prop - this has been update to use w = 100%.
• Button, Icon Button, Input, NumberInput, Radio, Checkbox, Select# We removed the isFullWidth prop from these components. Use width="full" or width="100%" instead.
• Please see Chakra docs for more info.

React Upgrades

• ReactDOM.render is no longer supported in React 18. We use createRoot instead
• No more package-lock.json. We are moving to yarn.lock with the use of yarn as package manager
• Reduced reliance on craco. We still need craco for running the test command. Otherwise, all other commands will use react-scripts
• Hot module reloading, which was previously broken, is now fixed in this PR

Visual Changes

Due to the upgrade of design system, the headings have been remapped.

Previous H6 are now h4
Previous H5 are now h3
Previous H4 are now h2

See the difference between these files to understand what has changed:

• v0.0.8: https://github.com/opengovsg/design-system/blob/v0.0.8/react/src/theme/textStyles.ts
• v0.0.12: https://github.com/opengovsg/design-system/blob/v0.0.12/react/src/theme/textStyles.ts

Storybook Updates

When running npm run build-storybook it outputs a weird error:

ERR! ModuleNotFoundError: Module not found: Error: Can't resolve '@mdx-js/react' in '/.storybook/foundations/Colours'

❯ npm run build-storybook

> [email protected] build-storybook
> build-storybook -s public

info @storybook/react v6.5.16
info
info => Cleaning outputDir: isomercms-frontend/storybook-static
(node:20070) DeprecationWarning: --static-dir CLI flag is deprecated, see:

https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#deprecated---static-dir-cli-flag
(Use `node --trace-deprecation ...` to show where the warning was created)
info => Loading presets
info => Copying static files: ./public => ./
info => Compiling manager..
info => Compiling preview..
info Addon-docs: using MDX1
info => Loading Webpack configuration from `node_modules/react-scripts`
info => Removing existing JavaScript and TypeScript rules.
info => Modifying Create React App rules.
info => Using default Webpack5 setup
ERR! => Failed to build the preview
ERR! Module not found: Error: Can't resolve '@mdx-js/react' in 'isomercms-frontend/.storybook/foundations/Colours'

There is a related issue for this: storybookjs/storybook#18094 which mentions some compatibility issues for storybook with react 18 version

I have performed some upgrades to overcome this and installed the missing deps as devDeps in package.json

Upgrade storybook with npx storybook upgrade
https://storybook.js.org/docs/react/configure/upgrading

. ✓
🔎 checking possible migrations..
Found builder webpack5, skipping
Found builder webpack5, skipping
🔎 found a 'eslintPlugin' migration:

╭──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                                                      │
│   We've detected you are not using our eslint-plugin.                                                                                │
│                                                                                                                                      │
│   In order to have the best experience with Storybook and follow best practices, we advise you to install eslint-plugin-storybook.   │
│                                                                                                                                      │
│   More info: https://github.com/storybookjs/eslint-plugin-storybook#readme                                                           │
│                                                                                                                                      │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
✔ Do you want to run the 'eslintPlugin' migration on your project? … yes
✅ Adding dependencies: eslint-plugin-storybook

added 3 packages, and audited 3859 packages in 26s

474 packages are looking for funding
  run `npm fund` for details

30 high severity vulnerabilities

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.
❌ error when running eslintPlugin migration:
⚠️ The plugin was successfuly installed but failed to configure.

Found an .eslintrc config file with an unsupported automigration format: json.
Supported formats for automigration are: js, cjs.

Please refer to https://github.com/storybookjs/eslint-plugin-storybook#usage to finish setting up the plugin manually.

🔎 found a 'npm7' migration:

╭───────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                   │
│   We've detected you are running npm 9.5.0                                                        │
│    which has peer dependency semantics which Storybook is incompatible with.                      │
│                                                                                                   │
│   In order to work with Storybook's package structure, you'll need to run `npm` with the          │
│   `--legacy-peer-deps=true` flag. We can generate an `.npmrc` which will do that automatically.   │
│                                                                                                   │
│   More info: https://github.com/storybookjs/storybook/issues/18298                                │
│                                                                                                   │
╰───────────────────────────────────────────────────────────────────────────────────────────────────╯
✔ Do you want to run the 'npm7' migration on your project? … yes
✅ ran npm7 migration

✅ migration check successfully ran

❯ npx storybook@next automigrate
Need to install the following packages:
  [email protected]
Ok to proceed? (y) y
🔎 checking possible migrations..
Found builder webpack5, skipping
Found builder webpack5, skipping

🔎 found a 'mdx1to2' migration:
╭ Automigration detected ──────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                                  │
│   We've found 1 '.stories.mdx' files in your project.                                                            │
│                                                                                                                  │
│   Storybook has upgraded to MDX2 (https://mdxjs.com/blog/v2/), which contains breaking changes from MDX1.        │
│   We can try to automatically upgrade your MDX files to MDX2 format using some common patterns.                  │
│                                                                                                                  │
│   After this install completes, and before you start Storybook, we strongly recommend reading the MDX2 section   │
│   of the 7.0 migration guide. It contains useful tools for detecting and fixing any remaining issues.            │
│                                                                                                                  │
│   https://storybook.js.org/migration-guides/7.0                                                                  │
│                                                                                                                  │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
✔ Do you want to run the 'mdx1to2' migration on your project? … yes
🆗 Unmodified Colours.stories.mdx
✅ ran mdx1to2 migration

🔎 found a 'autodocsTrue' migration:
╭ Automigration detected ────────────────────────────────────────────────────────────────────────────╮
│                                                                                                    │
│   We've changed the configuration of autodocs (previous docsPage), so now the value:               │
│     - docs.autodocs: true -- means automatically create docs for every CSF file                    │
│     - docs.autodocs: 'tag' -- means only create autodocs for CSF files with the 'autodocs' tag     │
│     - docs.autodocs: false -- means never create autodocs                                          │
│                                                                                                    │
│   Based on your prior configuration,  we can set the `docs.autodocs` to keep your old behaviour:   │
│                                                                                                    │
│   docs: { autodocs: true }                                                                         │
│                                                                                                    │
│   More info: https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#autodocs-changes      │
│                                                                                                    │
╰────────────────────────────────────────────────────────────────────────────────────────────────────╯
✔ Do you want to run the 'autodocsTrue' migration on your project? … yes
✅ Setting 'docs.autodocs' to true in main.js
✅ ran autodocsTrue migration

╭ Migration check ran successfully ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│                                                                                                                                                               │
│                                                                                                                                                               │
│         Successful migrations:                                                                                                                                │
│                                                                                                                                                               │
│    mdx1to2, autodocsTrue                                                                                                                                      │
│                                                                                                                                                               │
│       ─────────────────────────────────────────────────                                                                                                       │
│                                                                                                                                                               │
│       If you'd like to run the migrations again, you can do so by running 'npx storybook@next automigrate'                                                    │
│                                                                                                                                                               │
│   The automigrations try to migrate common patterns in your project, but might not contain everything needed to migrate to the latest version of Storybook.   │
│                                                                                                                                                               │
│   Please check the changelog and migration guide for manual migrations and more information: https://storybook.js.org/migration-guides/7.0                    │
│   And reach out on Discord if you need help: https://discord.gg/storybook                                                                                     │
│                                                                                                                                                               │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

Cypress Changes

Cypress is working good with webpack 5 now. Some issues and how I overcame them:

When there are errors such as cy.setSessionDefaults() is not defined

supportFile: "cypress/support/e2e.ts",

Reference

Key Changes

For Cypress, I had to refactor the imports in the tests. Starting from Webpack 5, process, path and other automatic polyfills were removed. Hence, we will encounter process is not defined error if we import files using env variables for example.

This is case, our spec files such as editPage.spec.js was import utils which in turn was importing api.js which in turn was using process.env to set some of the variables.

This was not unnecessary for Cypress tests. In fact, there should be no reliance on process.env for e2e tests.

Hence, I refactored all the required imports into a separate file called legacyUtils.js and then made more specific imports in our spec files to resolve this.

Related Reading:

• https://gist.github.com/ef4/d2cf5672a93cf241fd47c020b9b3066a

• https://stackoverflow.com/questions/65018431/webpack-5-uncaught-referenceerror-process-is-not-defined

• https://webpack.js.org/blog/2020-10-10-webpack-5-release/#changes-to-the-structure

Webpack Changes

Cypress Related

Inside cypress.config.ts, we use Webpack preprocessor 5 for Cypress to configure Webpack 5 compatibility.

Reference

Alias resolution for paths

https://webpack.js.org/configuration/resolve/

resolve: {
              alias: {
                utils: path.resolve(__dirname, "src/utils/index"),
              },
}

TypeScript Changes

Some TypeScript related changes were required for Cypress to work with Webpack. Check out the below issue and example for what worked.

Related reading: cypress-io/cypress#24185

on(
        'file:preprocessor',
        webpackPreprocessor({
          webpackOptions: {
            mode: 'development',
            // make sure the source maps work
            devtool: 'eval-source-map',
            // webpack will transpile TS and JS files
            resolve: {
              extensions: ['.ts', '.js'],
            },
            module: {
              rules: [
                {
                  // every time webpack sees a TS file (except for node_modules)
                  // webpack will use "ts-loader" to transpile it to JavaScript
                  test: /\.ts$/,
                  exclude: [/node_modules/],
                  use: [
                    {
                      loader: 'babel-loader',
                      options: {
                        presets: ['@babel/typescript'],
                      },
                    },
                  ],
                },
              ],
            },
          },
        })
      );

CI Changes

With the move to yarn, I had to edit all the workflow yml files we have. I also took this opportunity to upgrade the actions package versions we are using. The CI on this PR is expected to fail due to the numerous lint/formatting errors. This has been fixed in the next PR (see below section)

Related PR

There were a lot of Eslint and Prettier errors. To facilitate review, I have done them on a separate branch and PR here: #1203

The above PR will be merged into this current branch and then we will finally merge everything into identity-phase-2.

Tests

Before Testing

• Make sure you remove the existing node_modules folder
• Ensure you are using node version 18x. If not, use nvm to install node 18
• Do a yarn install

Tests to Run

Please run all tests including E2E tests to confirm everything is working fine.

• Unit tests (using npm run test)
• e2e tests (comment on this PR with the text !run e2e)
• Test visual changes via Staging CMS

Deploy Notes

Make sure this is well tested on staging before deploying to production.

[mergify]

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers

[kishore03109]

It was also able to resolve the peer dependencies without much issues.

Hmm I am confused here. Doesnt npm also easily resolve peer dependencies for you? like at most we can just run the npm download to install peer deps?


In fact, there should be no reliance on process.env for e2e tests.

This is not true right, we would need the GitHub access tokens for this? Additionally, this could be the case in the future right, how do we deal if that is the case?

There were some sourcemap generation warnings on service start. This seems to be an issue from Create React App and Webpack 5. See this open issue for more details.
Hence, for now I have disabled sourcemap generation in the dev command.

Hmm I am not too sure about this. I rather generate warning and fail quickly rather than disabling sourcemap as a whole during development

Other non-nits:
we should have an issue to use yarn in the backend!

[kishore03109]

Hey, why are we turning this off ah?

[harishv7]

There were quite a few typescript errors. I fixed most of them, except 1-2 which I had to ignore. I think if you look through below you would find 1-2 places where I am using ts-ignore to ignore the error for now.

@seaerchin mentioned he can help look into that

[kishore03109]

If there is only 1-2 places, could we mark them at the file level rather than having a global config here?

[harishv7]

It was also able to resolve the peer dependencies without much issues.

Hmm I am confused here. Doesnt npm also easily resolve peer dependencies for you? like at most we can just run the npm download to install peer deps?


In fact, there should be no reliance on process.env for e2e tests.

This is not true right, we would need the GitHub access tokens for this? Additionally, this could be the case in the future right, how do we deal if that is the case?

There were some sourcemap generation warnings on service start. This seems to be an issue from Create React App and Webpack 5. See this open issue for more details.
Hence, for now I have disabled sourcemap generation in the dev command.

Hmm I am not too sure about this. I rather generate warning and fail quickly rather than disabling sourcemap as a whole during development

Other non-nits: we should have an issue to use yarn in the backend!

1. Chatted offline on this
2. We should be using env vars through cypress.env.json or Cypress.env. https://docs.cypress.io/guides/guides/environment-variables
3. But firstly, how and what are we using sourcemaps for? Source maps allows to map a transformed file back to the original source file. If you think that's valuable, sure let's enable it
4. Yarn for BE issue: Update to use yarn isomercms-backend#661

[seaerchin]

dismissing my review cos it's stale but the type errors above are due to a version mismatch between the versions of design system and chakra.

we pinned design system to 0.0.12, which internally uses 2.1.x for chakra but we installed chakra at 2.5.x, which meant that the chakra type used by design system doesn't exist.

the path going forward (discussed offline w @harishv7) is to up design system version since we now satisfy peer deps but it means that we do have to resolve breaking changes ourselves

[kishore03109]

@harishv7

But firstly, how and what are we using sourcemaps for? Source maps allows to map a transformed file back to the original source file. If you think that's valuable, sure let's enable it

Hmm correct me if I am wrong ya, but source maps are being used for debugging purposes as it allows us to step through code based on the source code rather than the source code right? May I also have context to the warnings? does it warn that the sourcemaps couldn't be generated?

[harishv7]

@harishv7

But firstly, how and what are we using sourcemaps for? Source maps allows to map a transformed file back to the original source file. If you think that's valuable, sure let's enable it

Hmm correct me if I am wrong ya, but source maps are being used for debugging purposes as it allows us to step through code based on the source code rather than the source code right? May I also have context to the warnings? does it warn that the sourcemaps couldn't be generated?

@kishore03109

What do you mean by "step through code based on the source code rather than the source code right"?

Sourcemaps allow us to transform a file back into original source file for debugging purpose. Personally, I havent used it before, but I think let's keep it on for now.

Btw, I have re-enabled the sourcemaps, somehow after a clean install the warnings are gone. will continue monitoring this

[harishv7]

Hey team, I am unable to resolve this weird error. I had to unroll my shift to yarn for now to continue development. If you guys know of a way to resolve this weird error let me know.

❯ yarn dev
command not found: source

Technically we should not be using source for frontend scripts like this. For REACT_APP prefixed envs it should pick it up automatically. However, I felt this issue as a whole can be better tackled in a different PR to avoid mixing of too many things in 1 PR.

[kishore03109]

@harishv7

Upgrading to 1.x version seems non-trivial and should be tackled on a PR of its own. Example of errors we see with 1.2.0 is shown here:

oo got it, thanks for showing the examples as well

I understand that the nature of this PR will lead to having a lot of files changed. I am under the assumption that this PR took a pause to make sure e2e was in order before proceeding through the reviewal process again. Could I get your help to highlight key technical decisions made/non-trivial code changes that occurred from the time when eng took a pause from reviewing this to now so reviewers can focus specifically on code changes that has occurred since that pause?

[mergify]

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers