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

[editor-preview] remove unnecessary 'navigator' dependency #6648

Merged
merged 1 commit into from
Nov 28, 2019

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes #6639

  • removes the unused navigator from the editor-preview extension.
  • the navigator was needlessly pulled by the editor-preview as it was never used and is likely to be a leftover of 00e7d9d.

How to test

  • verify that CI passes (editor-preview builds and the tests pass)

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

Fixes #6639

- removes the unused `navigator` from the `editor-preview` extension.
- the `navigator` was needlessly pulled by the `editor-preview` as it was
never used and is likely to be a leftover of 00e7d9d.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
@vince-fugnitto vince-fugnitto added editor-preview issues that are related to the editor-preview dependencies pull requests that update a dependency file labels Nov 27, 2019
@vince-fugnitto vince-fugnitto self-assigned this Nov 27, 2019
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. A few notes:

  • I think the suggested test will not be conclusive, since the navigator is pulled (at least indirectly) from multiple Theia extensions used in the example application
  • I have manually checked that, indeed, the dependency looks unused ATM
  • removing the dependency will not practically change anything, but it will not hurt either, and its technically correct :)

@vince-fugnitto
Copy link
Member Author

LGTM. A few notes:

  • I think the suggested test will not be conclusive, since the navigator is pulled (at least indirectly) from multiple Theia extensions used in the example application

Yes you are right, a more exhaustive test would be to create an application that pulls the editor-preview without a navigator (which might be difficult at the moment due to coupling in other areas). I mentioned the test because I wanted to make sure that unit tests execute successfully.

  • I have manually checked that, indeed, the dependency looks unused ATM

👍

  • removing the dependency will not practically change anything, but it will not hurt either, and its technically correct :)

I think so too, I don't think we should have dependencies without the actual need for them, it makes the package.json of those such extensions confusing.

@vince-fugnitto vince-fugnitto merged commit 84d8678 into master Nov 28, 2019
@vince-fugnitto vince-fugnitto deleted the vf/GH-6639 branch November 28, 2019 12:21
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 editor-preview issues that are related to the editor-preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dependencies] 'editor-preview' needlessly pulls 'navigator'
2 participants