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

[Breaking changes] Bump several major dependencies #2041

Merged
merged 17 commits into from
Apr 9, 2021
Merged

Conversation

emmenko
Copy link
Member

@emmenko emmenko commented Feb 4, 2021

Follow up of #1874


This PR now contains several major dependency updates, resulting in releasing a new major of appkit due to the nature of peer dependencies and the major library upgrades.

More specifically:

  • React v17 is required now
  • Several other peer deps cleanups, mainly to drop old versions.
  • Using uikit v12
  • Using webpack v5
  • Using graphql v15
  • Using docskit v16 (gatsby v3)

The appkit packages don't contain any breaking changes.

@vercel
Copy link

vercel bot commented Feb 4, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/commercetools/merchant-center-application-kit/EHxwSkxdA6BGaavpSMW4Cg21aVuj
✅ Preview: https://merchant-center-application-kit-git-nm-webpack-5-commercetools.vercel.app

@changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2021

🦋 Changeset detected

Latest commit: b8777c2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 23 packages
Name Type
playground Major
@commercetools-frontend/create-mc-app Major
@commercetools-frontend/application-shell-connectors Major
@commercetools-frontend/mc-html-template Major
@commercetools-local/visual-testing-app Major
merchant-center-application-template-starter Major
@commercetools-frontend/l10n Major
@commercetools-frontend/permissions Major
@commercetools-frontend/react-notifications Major
@commercetools-frontend/actions-global Major
@commercetools-frontend/jest-stylelint-runner Major
@commercetools-frontend/mc-dev-authentication Major
@commercetools-frontend/sentry Major
@commercetools-frontend/jest-preset-mc-app Major
@commercetools-frontend/sdk Major
@commercetools-frontend/application-shell Major
@commercetools-frontend/application-components Major
@commercetools-frontend/application-config Major
@commercetools-frontend/i18n Major
@commercetools-frontend/mc-scripts Major
@commercetools-website/custom-applications Major
@commercetools-website/components-playground Major
@commercetools-frontend/cypress Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@emmenko
Copy link
Member Author

emmenko commented Feb 5, 2021

FYI: the production bundle seem to work but it's the dev server that has problems.

@emmenko
Copy link
Member Author

emmenko commented Mar 16, 2021

Note: re-enable website into workspaces once we are able to upgrade to Webpack v5

@emmenko
Copy link
Member Author

emmenko commented Mar 31, 2021

Update: things look good now, I was able to start dev server locally and to build the production app.

@vercel vercel bot temporarily deployed to Preview March 31, 2021 20:24 Inactive
Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

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

Ouuuuh very exciting! Thanks 🙇‍♀️

const formatWebpackMessages = require('react-dev-utils/formatWebpackMessages');
const FileSizeReporter = require('react-dev-utils/FileSizeReporter');
const printBuildError = require('react-dev-utils/printBuildError');
// const formatWebpackMessages = require('./patches/react-dev-utils/formatWebpackMessages');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this 🤔?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's redundant to L19

@@ -380,6 +379,8 @@ module.exports = function createWebpackConfigForDevelopment(options = {}) {
},
],
include: mergedOptions.sourceFolders.concat(vendorsToTranspile),
// Disable require.ensure as it's not a standard language feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@vercel vercel bot temporarily deployed to Preview April 8, 2021 10:17 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2021 11:26 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2021 14:12 Inactive
@emmenko emmenko changed the title [WIP] Migrate to Webpack 5 [Breaking changes] Bump several major dependencies Apr 8, 2021
@vercel vercel bot temporarily deployed to Preview April 8, 2021 14:25 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2021 14:55 Inactive
@emmenko emmenko marked this pull request as ready for review April 8, 2021 15:41
@emmenko emmenko requested review from tdeekens and adnasa April 8, 2021 15:41
@vercel vercel bot temporarily deployed to Preview April 8, 2021 15:48 Inactive
'playground': major
---

Ships with `react@17`
Copy link
Contributor

Choose a reason for hiding this comment

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

Now uses react@17 and react-dom@17.

'@commercetools-frontend/create-mc-app': major
---

- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Changes required Node.js engine version to `>=12 || >=14` in `package.json`.


- Requires a peer dependency of `react@17`.
- The `@types/react*` peer dependencies have been removed and included as normal dependencies with a minor range version.
- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Change required Node.js engine version to `>=12 || >=14` in `package.json`.

'@commercetools-frontend/mc-html-template': major
---

- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Change required Node.js engine version to `>=12 || >=14` in `package.json`.

'@commercetools-local/visual-testing-app': major
---

Ships with `react@17`
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
Ships with `react@17`
Ships with `react@17`Now uses react@17 and react-dom@17.

'merchant-center-application-template-starter': minor
---

- Ships template with `react@17`.
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
- Ships template with `react@17`.
Now uses react@17 and react-dom@17.

'@commercetools-frontend/mc-dev-authentication': major
---

- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Change required Node.js engine version to `>=12 || >=14` in `package.json`.

- The `@types/react*` peer dependencies have been removed and included as normal dependencies with a minor range version.
- The peer dependency `react-intl` now only requires version `>=5`.
- The peer dependency `@testing-library/react` now only requires version `>=11`.
- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Change required Node.js engine version to `>=12 || >=14` in `package.json`.

'@commercetools-frontend/mc-scripts': major
---

- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Change required Node.js engine version to `>=12 || >=14` in `package.json`.

'@commercetools-frontend/application-config': major
---

- Change required engine version to `>=12 || >=14`.
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
- Change required engine version to `>=12 || >=14`.
- Change required Node.js engine version to `>=12 || >=14` in `package.json`.

Copy link
Contributor

@adnasa adnasa left a comment

Choose a reason for hiding this comment

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

Let's spin this locally, one time.

Regarding the UI-kit upgrade, we'll run into conflicts.
either the ui-kit upgrade goes first or this PR. I'm fine with either, but perhaps it's easier that this PR goes first.

@@ -6,7 +6,7 @@
"private": true,
"scripts": {
"prepare": "husky install",
"postinstall": "manypkg check && preconstruct dev && yarn compile-css-modules && yarn --cwd website && yarn --cwd website-components-playground",
"postinstall": "manypkg check && preconstruct dev && yarn compile-css-modules",
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Comment on lines 109 to 113
if (result.errors.some(isLikelyASyntaxError)) {
// If there are any syntax errors, show just them.
result.errors = result.errors.filter(isLikelyASyntaxError);
}
return result;

This comment was marked as outdated.

const formatWebpackMessages = require('react-dev-utils/formatWebpackMessages');
const FileSizeReporter = require('react-dev-utils/FileSizeReporter');
const printBuildError = require('react-dev-utils/printBuildError');
// const formatWebpackMessages = require('./patches/react-dev-utils/formatWebpackMessages');
Copy link
Contributor

Choose a reason for hiding this comment

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

it's redundant to L19

Comment on lines 398 to 400
infrastructureLogging: {
level: 'warn',
level: 'none',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't the same as removing the option?

export type LinkProps = {
tone?: 'primary' | 'inverted';
isExternal?: boolean;
to?: string | H.LocationDescriptor;
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 yesssssssss

There is a PR for Link to TS on ui-kit..
this won't last long

Comment on lines 15 to 24
"@commercetools-uikit/design-system": "^11.2.1",
"@commercetools-uikit/icon-button": "^11.2.1",
"@commercetools-uikit/icons": "^11.2.1",
"@commercetools-uikit/multiline-text-field": "^11.3.0",
"@commercetools-uikit/secondary-button": "^11.2.1",
"@commercetools-uikit/select-field": "^11.2.1",
"@commercetools-uikit/spacings": "^11.2.1",
"@commercetools-uikit/text": "^11.2.1",
"@commercetools-uikit/text-field": "^11.2.1",
"@commercetools-uikit/text-input": "^11.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

okay we will have conflicts.
I'm fine with merging this PR first then updating ui-kit on top of that.

* chore: upgrade ui-kit

* refactor(types): add new horizontalConstraint map, add `design-system` module

* refactor(playground): remove old use of props

* refactor(application-components): remove use of old props

* refactor(visual-testing-app): remove use of old props

* chore: changeset

* chore: upgrade to latest docs-kit (includes uikit v12)

* fix: remove unnecessary types, fix constraints

Co-authored-by: Nicola Molinari <nicola.molinari@commercetools.de>
@vercel vercel bot temporarily deployed to Preview April 9, 2021 13:42 Inactive
Copy link
Contributor

@adnasa adnasa left a comment

Choose a reason for hiding this comment

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

🚀

@vercel vercel bot temporarily deployed to Preview April 9, 2021 14:39 Inactive
@emmenko emmenko merged commit a240f65 into main Apr 9, 2021
@emmenko emmenko deleted the nm-webpack-5 branch April 9, 2021 15:15
@emmenko emmenko mentioned this pull request Apr 9, 2021
@ghost ghost mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants