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

build(deps): update core deps #1054

Closed
wants to merge 13 commits into from
Closed

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Sep 13, 2022

NOTE: PLEASE DELETE NODE MODULES AND DO A CLEAN INSTALL

Problem

Isomer is unable to use cool features from design system due to lacking the appropriate peer deps. This restricts engineering capability as we are either unable to build to design specs or we have to find workarounds when a ready made solution is available in design system.

this PR also fixes hmr!!!

Solution

  1. update chakra 1.x to chakra 2.x
  2. update react 16 to react 18
  3. update design system to latest
  4. update react scripts to react scripts 5
  5. update sentry to latest 7.x
  6. update storybook config
  7. update storybook to 6.5

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible
  1. updating to chakra 2.x removes the isFullWidth prop - this has been update to use w = 100%.
  2. updating from react 16 to react 18 uses a new api for rendering index.js (details available here); this also removes implicit children so if there's a complain about children, this means the type needs to be wrapped in PropsWithChildren
  3. updating to latest storybook version uses webpack 5 under the hood. unfortunately, storybook docgen uses webpack 4 so we need to pin this in our devDeps (see here for details)
  4. removed storybook-preset-craco from addons for storybook config and replaced w/ @storybook/preset-create-react-app; the craco preset doesn't work with storybook possibly due to webpack 5 being used in storybook's new version.

Visual inconsistencies

  1. hover state for folder/ungrouped cards on Workspace lacks border colour (might be true for other cards in Resources and Media)
  2. contextMenu colour for items is wrong for the cards ^
    fyi @NatMaeTan @ogpgerald

Manual verification
the commands below should still work without any issues.

please run npm ci prior to running these commands!!!

  1. npm run dev
  2. npm run build-storybook (required for chromatic deployment)
  3. npm run storybook
  4. npm run test-e2e
  5. npm run cypress:open

src/components/contact-us/Section.jsx Outdated Show resolved Hide resolved
package.json Outdated
"@opengovsg/design-system-react": "0.0.8",
"@sentry/react": "^5.30.0",
"@opengovsg/design-system-react": "0.0.12",
"@sentry/react": "^7.12.1",
"@sentry/tracing": "^6.15.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be upgraded too? From their migration docs:

If you installed additional Sentry packages, such as@sentry/tracing alongside your Sentry SDK (e.g. @sentry/react or @sentry/node), make sure to upgrade all of them to version 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! i only saw the issue w/ the corresponding PR and just ran npm i so i missed this. thanks for the catch!

Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

The changes look ok to me, but since this is quite a significant change in our packages, may I get your help to run e2e on this branch? I would feel safer approving this after running them on Cypress!

@@ -18,14 +18,18 @@ export const get = async ({
export const update = async (
siteName: string,
// eslint-disable-next-line camelcase
{ facebook_pixel, linkedin_insights, ...rest }: BackendSiteSettings
{
facebook_pixel: pixel,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there a reason why we are using snake_case here?
If it is low effort, can we shift to camelCase instead? Else, I understand this would be out of the scope of this PR!

Copy link
Contributor Author

@seaerchin seaerchin Sep 15, 2022

Choose a reason for hiding this comment

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

we can't, because chromatic will fail with an eslint error here complaining about none camel cased variables. you can try this for yourself (run npm run build-storybook) HAHAHAHA

@seaerchin
Copy link
Contributor Author

The changes look ok to me, but since this is quite a significant change in our packages, may I get your help to run e2e on this branch? I would feel safer approving this after running them on Cypress!

hmm i actually omitted e2e purely because build level stuff won't fail at run time but instead, will fail to even compile. nevertheless, this is a valid point! i'll run e2e tests (and verify) for this PR

@seaerchin seaerchin temporarily deployed to staging September 15, 2022 12:51 Inactive
@seaerchin seaerchin temporarily deployed to staging September 15, 2022 13:03 Inactive
@seaerchin seaerchin temporarily deployed to staging September 15, 2022 13:04 Inactive
chore(workspace): removed wip ode

fix(button): changed buttons that have the isFullWidth prop to using w=100%
this is NOT WORKING due to storybook using postcss7, whereas some stuff requires postcss 8; will
attempt upgrade to postcss 8 using storybook
@seaerchin seaerchin temporarily deployed to staging September 15, 2022 13:15 Inactive
@seaerchin seaerchin force-pushed the build/update-core-deps branch from 17071c4 to bd91a19 Compare September 19, 2022 06:32
@seaerchin seaerchin temporarily deployed to staging September 19, 2022 06:32 Inactive
@seaerchin
Copy link
Contributor Author

gonna close this PR first to avoid pollution; can't find a way to make cypress work w/ webpack 5 so i think it's better to have e2e in teh meantime when we're trying to get features out rather than start from scratch (using playwright) or omitting e2e totally

@seaerchin seaerchin closed this Sep 19, 2022
@seaerchin seaerchin temporarily deployed to staging September 19, 2022 06:45 Inactive
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.

3 participants