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

Adds missing dependencies #40165

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Adds missing dependencies #40165

merged 1 commit into from
Mar 19, 2020

Conversation

scinos
Copy link
Contributor

@scinos scinos commented Mar 16, 2020

Changes proposed in this Pull Request

  • Adds missing dependencies to package.json, and removes unused deps.
  • Detected by https://www.npmjs.com/package/dependency-check (running for file in $(find packages -type d -depth 1 | sort); do (cd $file; echo $file; dependency-check --no-peer --no-dev --missing package.json dist/**/*.js; echo) ; done

Before:

packages/babel-plugin-i18n-calypso

packages/babel-plugin-transform-wpcalypso-async

packages/calypso-analytics

packages/calypso-build

packages/calypso-codemods

packages/calypso-color-schemes

packages/calypso-polyfills

packages/components
Fail! Dependencies not listed in package.json: @storybook/addon-actions, components, devdocs

packages/composite-checkout
Fail! Dependencies not listed in package.json: @babel/runtime

packages/composite-checkout-wpcom
Fail! Dependencies not listed in package.json: @automattic/calypso-polyfills

packages/data-stores

packages/effective-module-tree

packages/eslint-config-wpcalypso

packages/eslint-plugin-wpcalypso

packages/format-currency
Fail! Dependencies not listed in package.json: @babel/runtime

packages/i18n-calypso
Fail! Dependencies not listed in package.json: @babel/runtime

packages/i18n-calypso-cli

packages/load-script

packages/material-design-icons

packages/media-library

packages/photon
Fail! Dependencies not listed in package.json: @babel/runtime

packages/react-i18n

packages/tree-select

packages/viewport

packages/viewport-react
Fail! Dependencies not listed in package.json: @babel/runtime

packages/webpack-config-flag-plugin

packages/webpack-extensive-lodash-replacement-plugin

packages/webpack-inline-constant-exports-plugin

packages/wpcom-proxy-request
Fail! Dependencies not listed in package.json: @babel/runtime

packages/wpcom.js

After:

packages/babel-plugin-i18n-calypso

packages/babel-plugin-transform-wpcalypso-async

packages/calypso-analytics

packages/calypso-build

packages/calypso-codemods

packages/calypso-color-schemes

packages/calypso-polyfills

packages/components
Fail! Dependencies not listed in package.json: @storybook/addon-actions, components, devdocs

packages/composite-checkout

packages/composite-checkout-wpcom

packages/data-stores

packages/effective-module-tree

packages/eslint-config-wpcalypso

packages/eslint-plugin-wpcalypso

packages/format-currency

packages/i18n-calypso

packages/i18n-calypso-cli

packages/load-script

packages/material-design-icons

packages/media-library

packages/photon

packages/react-i18n

packages/tree-select

packages/viewport

packages/viewport-react

packages/webpack-config-flag-plugin

packages/webpack-extensive-lodash-replacement-plugin

packages/webpack-inline-constant-exports-plugin

packages/wpcom-proxy-request

packages/wpcom.js

Testing instructions

  • Validate that all* dependencies are satisfied now (for file in $(find packages -type d -depth 1 | sort); do (cd $file; echo $file; dependency-check --no-peer --no-dev --missing package.json dist/**/*.js; echo) ; done)

  • Validate that no new dependencies are actually introduced (there are no new packages in package-lock.json), just re-linking existing packages.

@scinos scinos requested review from a team as code owners March 16, 2020 11:52
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@scinos
Copy link
Contributor Author

scinos commented Mar 16, 2020

packages/components is still missing some dependencies but they are only used in storybooks and examples, and dependency-check gets a bit confused.

@scinos scinos self-assigned this Mar 16, 2020
@scinos scinos added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 16, 2020
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This looks good 👍 Thanks!

Should we also add @automattic/calypso-build to some of those? It looks like we're using its tsconfig and jest config preset in some of the packages?

@sirreal
Copy link
Member

sirreal commented Mar 17, 2020

It looks like we're using its tsconfig and jest config preset in some of the packages?

I believe these would both be considered dev dependencies so should be coming from the root. We don't need them to be bundled with the package.

Aside: I'd like to remove the tsconfig from calypso-build. I think it was premature to add it there, we should just have a tsconfig for the monorepo.

@tyxla
Copy link
Member

tyxla commented Mar 17, 2020

I thought that would be the case. 👍 In that case, I think we're good to go here.

Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

👍 I don't like the calypso-polyfills dependency, but that doesn't prevent merging 🚢

@scinos scinos merged commit 723973c into master Mar 19, 2020
@scinos scinos deleted the fix/add-missing-dependencies branch March 19, 2020 07:21
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 19, 2020
@scinos scinos mentioned this pull request Apr 8, 2020
11 tasks
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.

None yet

6 participants