-
Notifications
You must be signed in to change notification settings - Fork 148
fix: update dependencies to address security issue in @babel/runtime < 7.26.10 #375
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
base: main
Are you sure you want to change the base?
Conversation
|
I know this was a huge overhaul, but would love to get the cli tool up to date 🙏 |
|
Bump 🙏 |
| steps: | ||
| - prep_env | ||
| - run: yarn install | ||
| - run: pnpm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool - i didn't know about pnpm!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we moved from yarn --> npm for the documentation i wonder if we should use this over in that repo too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I personally love pnpm and use it in all my projects (template: create-typescript-app), switching this CLI from a previous package manager to pnpm is a big change. If we want to do that (+1 from me) it should be in a separate PR than a smaller "update dependencies" one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
|
@all-contributors please add @jdalrymple for security, bug |
|
I've put up a pull request to add @jdalrymple! 🎉 |
|
hey @jdalrymple thank you for this - this is a BIG PR!! We have been working on reviving this project and I do have merge permissions but i'm trying to use them carefully as we all get up to speed on this project. Do you have the capacity to split this out into small pieces? I am doing updates such as dependabot etc as well. if you are willing to work with us, implementing this in several smaller pr's would be greatly appreciated! |
| @@ -1,15 +1,23 @@ | |||
| version: 2.1 | |||
|
|
|||
| orbs: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think i have access to circle ci so let me dig into that first. if this could be it's own pr we can merge it quickly as is
|
maybe one pr for circle ci i am still trying to figure out how deployment is setup here. And where tests run. lemme know how that sounds. I appreciate your help!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 I appreciate the initiative! There are quite a few unrelated changes in this PR ranging from stylistic tweaks in other files to a full swap of the package manager. +1 to lwasser's suggestion to have one PR per thing. That makes things much more reviewable. And if one item is blocked, that doesn't block the other items. Thanks!
| steps: | ||
| - prep_env | ||
| - run: yarn install | ||
| - run: pnpm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I personally love pnpm and use it in all my projects (template: create-typescript-app), switching this CLI from a previous package manager to pnpm is a big change. If we want to do that (+1 from me) it should be in a separate PR than a smaller "update dependencies" one.
| @@ -1,5 +1,4 @@ | |||
| const url = require('url') | |||
| const fetch = require('node-fetch') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving from node-fetch to the global fetch is also a bigger change than just bumping @babel/runtime. node-fetch isn't quite a drop-in replacement for fetch. fetch is more spec/standards-compliant.
| if (commitConvention.lowercase) | ||
| if (commitConvention.lowercase) { | ||
| commitMessage = commitConvention.transform(commitMessage) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Style] Unrelated change - this isn't a problem on its own, but with all the other unrelated changes makes this PR harder to review.
|
Hey folks - I know this is a gigantor PR haha, i think in the process of updating the dep, I got ahead of myself cleaning up things i saw along the way. I can split it into multiple PRs for sure. Ill start with the CI updates and pick the next piece after that. Ill keep this PR open for now just to keep a reference of the direction we were trying to go in. Thanks for following up on everything 🙏 !! |
|
Thank you so much for breaking it up. We really appreciate it.
It is taking us some time to get up to speed as this is a large project and we are still gaining access to accounts!
Happy to review pr's as they come in. I did merge one dependabot update so you may need to rebase. We can expect ci to fail for now - i'm going to migrate us over to github actions soon.
… Message ID: ***@***.***>
|
| expect(data).toStrictEqual(file) | ||
| return unlink(file, err => { | ||
| if (err) throw err | ||
| expect(err).toBeNull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied this here: #396
What:
Modernized project dependencies and resolved deprecated packages, peer dependency warnings, and security vulnerabilities.
Why:
How:
Dependency Updates:
Configuration Changes:
Code Updates:
Infrastructure Updates:
.yvmrcfile (Yarn version specification no longer needed)Checklist: