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: pin colors to 1.4.0 #3995

Merged
merged 1 commit into from
Jan 10, 2022
Merged

fix: pin colors to 1.4.0 #3995

merged 1 commit into from
Jan 10, 2022

Conversation

erezrokah
Copy link
Contributor

πŸŽ‰ Thanks for submitting a pull request! πŸŽ‰

Summary

Fixes #3981

We don't use colors as a direct dependency, however by installing it as one, we can force package managers to use the unbroken version.

npm ls colors

β”œβ”€β”€ colors@1.4.0
β”œβ”€β”¬ node-version-alias@1.0.1
β”‚ └─┬ all-node-versions@8.0.0
β”‚   └─┬ fetch-node-website@5.0.3
β”‚     └─┬ cli-progress@3.9.1
β”‚       └── colors@1.4.0 deduped
β”œβ”€β”¬ prettyjson@1.2.1
β”‚ └── colors@1.4.0 deduped
└─┬ winston@3.3.3
  └─┬ logform@2.3.0
    └── colors@1.4.0 deduped

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’». This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)
πŸ’

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Jan 10, 2022
@github-actions
Copy link

πŸ“Š Benchmark results

Comparing with 443cc60

Package size: 356 MB

⬆️ 0.00% increase vs. 443cc60

^  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  359 MB  356 MB 
β”‚   β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”    β”Œβ”€β”€β”  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
β”‚   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |β–’β–’|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@erezrokah erezrokah added the automerge Add to Kodiak auto merge queue label Jan 10, 2022
Copy link
Collaborator

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

LGTM!

@erezrokah erezrokah merged commit 28450c1 into main Jan 10, 2022
@erezrokah erezrokah deleted the fix/pin_colors branch January 10, 2022 10:17
@XhmikosR
Copy link
Contributor

I don't think this PR fixes the issue... Since this project is using a lock file, this add a redundant dependency which does not enforce the package managers to respect it in any way.

One solution would be to use overrides and resolutions with whatever downsides they have.

The best solution would be to bug the direct parents of colors to pin the dependency on their side, until npm finally removes the offending packages.

@erezrokah
Copy link
Contributor Author

This is a not ideal solution for yarn. With npm we didn't encounter the issue as we have a shrinkwrap file (which yarn doesn't support).
The idea is to rely on package managers de-deduping mechanism to install the correct version.

One solution would be to use overrides and resolutions with whatever downsides they have.

Those work at the consumer level, and not at the publisher level as far as I know (overrides is quite new as well and only supported in latest npm).

The best solution would be to bug the direct parents of colors to pin the dependency on their side, until npm finally removes the offending packages.

πŸ’― And we'll follow up with that.

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 10, 2022

The idea is to rely on package managers de-deduping mechanism to install the correct version.

But this change here does not do this. You can try it and you'll see that whatever you have in the root project as a dependency, is ignored downlevel.

@erezrokah
Copy link
Contributor Author

Oh well, maybe because I didn't really pin it. Testing #3998

@XhmikosR
Copy link
Contributor

@erezrokah I think you can update to the latest prettyjson which has colors now pinned too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error on post install script
3 participants