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

[bunyan] remove '@theia/bunyan' extension #6651

Merged
merged 2 commits into from
Dec 2, 2019
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Nov 27, 2019

What it does

Fixes #6637

Motivation:

  • the extension is not used by any other extension present in the main repo.
  • the extension is not part of the example-browser or example-electron application, therefore it cannot be tested properly.
  • since it is not part of any other extension, or tested, it is likely to be un-maintained.
  • removing the extension also has a positive side-effect on build and CI time

How to test

  • the PR was tested by verifying if any @theia/bunyan dependencies were present, building the project and ensuring that tests successfully passed when removed.

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Nov 27, 2019

I also noticed the dependency bunyan and @types/bunyan are declared but unused at the moment, should I clean those up as well?

"@types/bunyan": "^1.8.0",

┌─[±][vf/GH-6637 ✓][theia][]
└─▪ yarn why "bunyan"
yarn why v1.17.3
[1/4] 🤔  Why do we have the module "bunyan"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#@theia#application-manager" depends on it
   - Hoisted from "_project_#@theia#application-manager#bunyan"
info Disk size without dependencies: "496KB"
info Disk size with unique dependencies: "496KB"
info Disk size with transitive dependencies: "496KB"
info Number of shared dependencies: 0
✨  Done in 1.01s.

CHANGELOG.md Outdated Show resolved Hide resolved
@vince-fugnitto
Copy link
Member Author

@marcdumais-work I added an extra commit which takes care of removing the bunyan and @types/bunyan dependencies from application-manager and core respectfully.

@marcdumais-work
Copy link
Contributor

Looking good - with the latest version, the only bunyan reference I find when grepping the repo is from CHANGELOG:

gitpod /workspace/theia $ grep -R -i bunyan *
CHANGELOG.md: - [bunyan] removed @theia/bunyan extension #6651

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

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

LGTM, but we should probably wait at least a day before merging, to give community members time to potentially object.

@vince-fugnitto
Copy link
Member Author

LGTM, but we should probably wait at least a day before merging, to give community members time to potentially object.

Sounds good! :)

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I'm happy to remove unused stuff :)

@akosyakov
Copy link
Member

akosyakov commented Nov 28, 2019

LGTM, but we should probably wait at least a day before merging, to give community members time to potentially object.

Maybe announce on Spectrum, just to make sure. But we can always reincarnate it as a separate repo if there anyone eager to maintain it.

@akosyakov akosyakov added the dependencies pull requests that update a dependency file label Nov 28, 2019
@vince-fugnitto vince-fugnitto force-pushed the vf/GH-6637 branch 3 times, most recently from f0ee034 to f6ec01a Compare November 28, 2019 11:44
@vince-fugnitto
Copy link
Member Author

LGTM, but we should probably wait at least a day before merging, to give community members time to potentially object.

Maybe announce on Spectrum, just to make sure. But we can always reincarnate it as a separate repo if there anyone eager to maintain it.

I created a post on Spectrum as well so its more visible to the community: https://spectrum.chat/theia/general/the-theia-bunyan-extension-to-be-removed~811cbba0-6977-449b-b7b0-ac5fb25b9231

Fixes #6637

Motivation:
- the extension is not used by any other extension present in the main repo.
- the extension is not part of the `example-browser` or `example-electron`
application, therefore it cannot be tested properly.
- since it is not part of any other extension, or tested, it is likely
to be un-maintained.

Signed-off-by: vince-fugnitto <[email protected]>
- removes the `bunyan` dependency which is unused.
- removes the `@types/bunyan` dependency which is unused.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

I'll give others a chance to view the PR if they'd like and merge on Monday.

@vince-fugnitto vince-fugnitto merged commit b76f08e into master Dec 2, 2019
@vince-fugnitto vince-fugnitto deleted the vf/GH-6637 branch December 2, 2019 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] why is '@theia/bunyan' part of the main repository
3 participants