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

Update many dependencies #133

Merged
merged 14 commits into from
Aug 8, 2024
Merged

Update many dependencies #133

merged 14 commits into from
Aug 8, 2024

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented May 29, 2024

These were some low-hanging fruit for upgradable dependencies.

(And some slightly higher-hanging fruit, TBH, see the custom resolutions in package.json)

Might as well, if we can, right?

Tests pass on my machine, and I can still install packages. (I tried language-carp and atom-ide-base; Both had no obvious issues installing, in terms of being able to run ppm install language-carp or ppm install atom-ide-base without running into any errors on the CLI, at least.)

Additional manual testing may be desirable, if we can get folks with the knowledge and time to thoroughly do so. Otherwise, we may just have to ask people to test it in the wild if we want to update these deps. In which case, best to have it in Rolling for a while and put out a call for testing there. Otherwise, we can perhaps try to call for testing of ppm itself with these changes before shipping it with Pulsar. But in any case, updating these seems like a good idea, IMO.

Note: For documentation of Yarn's "resolutions" system, see: https://classic.yarnpkg.com/en/docs/selective-version-resolutions

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Here is my light approval.

Looking at just code wise, everything here seems to be totally fine, with the biggest change in deps being plist which seems to only have any control in package-converted and text-mate-theme which if memory serves only effect migrating a TextMate theme to a Pulsar TextMate theme, which already is something we have talked about removing before, so it's functionality may not be a top priority.

Otherwise for some of these dependencies, I'd hope that automated testing would catch any issues present, but admittedly haven't found time to test. But I wanted to leave a review so that in any case we can merge this change, and let more rigorous testing occur in rolling, which would already be the case, since otherwise I don't see anything too obvious here that should give us any issue.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Aug 8, 2024

Gonna go ahead and merge this. Thanks for the review, even if it's a lite-approve I'm sure ppm will get testing to reveal if there are any problems before long. Besides that I used it for some things myself and it went okay.

@DeeDeeG DeeDeeG merged commit a5930b4 into master Aug 8, 2024
11 checks passed
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.

2 participants