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

[core] Cleanup Unused Deps #771

Merged
merged 3 commits into from
Oct 17, 2023
Merged

[core] Cleanup Unused Deps #771

merged 3 commits into from
Oct 17, 2023

Conversation

confused-Techie
Copy link
Member

While investigating our dependencies I've noticed there are a few that don't seem to be used anywhere, that we should be fine to go ahead and remove.

  • functional-red-black-tree: This seems to have been introduced prior to being entirely transpiled over to ./src/rb-tree.js. Meaning it's never actually been required into the project.
  • property-accessors: Seems to have been removed back in 2015 via 69833af although for some reason still appears in the repo. I can't find any reference to it anywhere, and as it's a mixin, I think we should be safe to remove.
  • glob: Originally introduced via fa90851 when adding benchmarking functionality to Atom, but since then benchmarking mode has been totally removed from Pulsar, and this file is not required anywhere else in the codebase.

So as long as tests all pass, we should be good to remove these dependencies as they are otherwise useless.

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 17, 2023

property-accessors: Seems to have been removed back in 2015 via 69833af although for some reason still appears in the repo. I can't find any reference to it anywhere, and as it's a mixin, I think we should be safe to remove.

"Somehow, property-accessors returned."

Memes aside, this description made me suspect a weird artifact of merge conflict resolution -- someone skims the changes and doesn't notice one line of package.json that they're accidentally re-introducing when incorrectly resolving the merge conflict.

I did a git bisect to track down where this might have happened. And... c148df11c9cbc19ef27412e858ddcebf4dd8e737.

So yeah, looks like an oversight during a merge.

@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks for tracking that down! I was a little worried when I was unable to find it, but that makes total sense. The commit you've linked was just the next day, so seems super probably they just had an older version checked out when making those changes, and that one modification got missed.

Glad to know that we can in fact remove this one then

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I looked around (especially with git grep [package name here]) and couldn't find any obvious uses of these packages as top-level dependencies in the same context as the main package.json.

(Some packages have their own indirect dependencies on other copies of glob or property-accessors, but those shouldn't be overlapping with or related to this stuff.)

Seems legit.

EDIT to add: Nice to lighten up the dependencies a little bit, every bit helps.

@confused-Techie
Copy link
Member Author

Thanks for the approval @DeeDeeG!

@confused-Techie confused-Techie merged commit addb8ee into master Oct 17, 2023
99 checks passed
@confused-Techie confused-Techie deleted the cleanup branch October 17, 2023 04:12
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