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

colors.js has issues #251

Merged
merged 2 commits into from
Jan 9, 2022
Merged

colors.js has issues #251

merged 2 commits into from
Jan 9, 2022

Conversation

JJ
Copy link
Contributor

@JJ JJ commented Jan 9, 2022

Mainly, this Marak/colors.js#285 Latest version has been compromised. A former maintainer, @DABH, has released this alternative 1.4.0 version.
Alternatively, it could simply be eliminated, since it's optional.

Mainly, this Marak/colors.js#285 Latest version has been compromised. A  former maintainer, @DABH, has released this alternative 1.4.0 version
@DanielRuf
Copy link
Contributor

We should pin it. See also Marak/colors.js#285 (comment) for some easy workarounds in your projects.

@JJ
Copy link
Contributor Author

JJ commented Jan 9, 2022

We should pin it. See also Marak/colors.js#285 (comment) for some easy workarounds in your projects.

Both are valid, according to [this comment]8Marak/colors.js#285 (comment)) by @DABH. However, in that same comment they're committing to keep upgrading so it might be a better option. Anyway, your call, and all my support for anything you decide.

@DanielRuf
Copy link
Contributor

However, in that same comment they're committing to keep upgrading so it might be a better option

But this will not fix the current releases of cli-table3.

If you can provide a PR to pin the version, we can merge this and make a new release.

But previous releases will still be affected. That's why we will have to document resolutions and patch-package as possible solutions.

And we should wait until next week / Monday to see which route to go with the package in general. Best would be an org, which takes over the maintenance of colors.

@DanielRuf
Copy link
Contributor

If you can provide a PR to pin the version, we can merge this and make a new release.

That's generally a better option to pin it to 1.4.0 since old package releases with so many installs and packages in general can not be deleted anymore after the left-pad case happened.

Please bear in mind that, since this points to the old repo that's still compromised, it's not impossible to re-tag a new commit. So I think it's a good compromise solution, but probably not the best going forward. But then, that's the bread and butter of development, isn't it?
@DanielRuf
Copy link
Contributor

@Turbo87 hi, can you merge and prepare / push a new release, when you find some time? That would be great.

@Turbo87
Copy link
Contributor

Turbo87 commented Jan 9, 2022

@DanielRuf currently a bit busy here. I've invited you to the org and added you on npm. feel free to merge and release :)

@DanielRuf
Copy link
Contributor

@DanielRuf currently a bit busy here. I've invited you to the org and added you on npm. feel free to merge and release :)

Thanks, I have accepted the invitation.

Only those with write access to this repository can merge pull requests.

Hm, seems I need a few more rights.
When I'm on a computer later I will make a release using GH Codespaces, let's see how this will work.

@DanielRuf
Copy link
Contributor

As I still need write access to the repo I did this:

  • create GitHub Codespaces instance from this PR
  • update yarn.lock
  • bump the version in package.json
  • publish to npm as 0.6.1

You can see the changes also at https://diff.intrinsic.com/cli-table3/0.6.0/0.6.1

Pushing the changes to the fork in a few minutes.

@DanielRuf
Copy link
Contributor

@JJ did you check / enable the checkbox in you PR to allow changes by us?

Because I get this on "git push":

remote: Permission to JJ/cli-table3.git denied to DanielRuf.
fatal: unable to access 'https://github.com/JJ/cli-table3/': The requested URL returned error: 403

@DanielRuf DanielRuf merged commit 75fa69f into cli-table:master Jan 9, 2022
@JJ
Copy link
Contributor Author

JJ commented Jan 9, 2022

@JJ did you check / enable the checkbox in you PR to allow changes by us?

Because I get this on "git push":

remote: Permission to JJ/cli-table3.git denied to DanielRuf.
fatal: unable to access 'https://github.com/JJ/cli-table3/': The requested URL returned error: 403

Do you still need this? I might have not. Anyway, I guess you'll make any changes afterwards. Thanks!

@DanielRuf
Copy link
Contributor

Do you still need this? I might have not. Anyway, I guess you'll make any changes afterwards. Thanks!

Thanks, it is not needed anymore.
I found a workaround and have now the needed write permissions for the project.

Thanks for your contribution and help with this.

jan-molak added a commit to serenity-js/serenity-js that referenced this pull request Jan 10, 2022
…onal) colors.js

cli-table3 pins its (optional) dependency on colors.js to 1.4.0;
While Serenity/JS doesn't use colors.js, older versions of NPM will install this optional dependency by default.
This change to Serenity/JS dependencies ensures that even if the optional dependency of cli-table3 gets installed, the user gets a safe version.

For details see:
cli-table/cli-table3#251
Marak/colors.js#285
Marak/colors.js#285 (comment)
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

3 participants