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

Fix cyclic dependency that will cause pcln-icons tests to fail in future #995

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

craigpalermo
Copy link
Collaborator

@craigpalermo craigpalermo commented Dec 28, 2020

While working on #970, I noticed that pcln-icons tests failed after upgrading to pcln-design-system@^4.2.1. It was previously on pcln-design-system@^3, and upgrading seemed to cause an inexplicable error when running rush update related to an already existent symlink to packages/icons/node_modules/pcln-design-system/dist/cjs/index.js.

This PR includes the following changes:

  • Upgrade to latest Rush and PNPM
  • Use PNPM workspaces to resolve cyclic dependency issues
  • Upgrade to latest pcln-design-system in devDeps of pcln-icons
  • Update allowed peerDep of react and react-dom in babel-preset to match that of the rest of the packages in the repo. This mismatch was causing a React invalid hooks error when running tests for pcln-icons because there would be multiple versions of React in the dependency tree.
  • Move require for @rushstack/eslint-patch to individual project .eslintrc.js files as recommended in their README

@craigpalermo craigpalermo added bug dependencies Pull requests that update a dependency file labels Dec 28, 2020
@craigpalermo craigpalermo force-pushed the fix-icons-tests-cyclic-dep branch 2 times, most recently from ec27405 to a332f28 Compare December 28, 2020 20:10
@codecov-io
Copy link

codecov-io commented Dec 28, 2020

Codecov Report

Merging #995 (09e02ff) into master (1af0b8d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #995   +/-   ##
=======================================
  Coverage   91.94%   91.94%           
=======================================
  Files          86       86           
  Lines        1478     1478           
  Branches      287      287           
=======================================
  Hits         1359     1359           
  Misses         98       98           
  Partials       21       21           
Impacted Files Coverage Δ
packages/icons/src/Svg.js 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1af0b8d...63d7027. Read the comment docs.

@craigpalermo craigpalermo marked this pull request as ready for review December 28, 2020 20:17
@craigpalermo craigpalermo force-pushed the fix-icons-tests-cyclic-dep branch 3 times, most recently from 4ae8664 to cb460c8 Compare December 30, 2020 15:37
Copy link
Collaborator

@sdalonzo sdalonzo left a comment

Choose a reason for hiding this comment

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

As discussed, waiting to see the rush-lib bump as well as seeing if the snapshots calm down after a rebase.

@craigpalermo craigpalermo force-pushed the fix-icons-tests-cyclic-dep branch from cb460c8 to efa34f6 Compare January 5, 2021 19:46
@craigpalermo craigpalermo mentioned this pull request Jan 5, 2021
@craigpalermo craigpalermo force-pushed the fix-icons-tests-cyclic-dep branch 4 times, most recently from 09e02ff to d751f73 Compare January 5, 2021 21:39
@craigpalermo craigpalermo force-pushed the fix-icons-tests-cyclic-dep branch from d751f73 to 63d7027 Compare January 5, 2021 21:46
@craigpalermo craigpalermo merged commit a48cb6e into master Jan 6, 2021
@craigpalermo craigpalermo deleted the fix-icons-tests-cyclic-dep branch January 6, 2021 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants