Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
modules/performance: add an option to combine plugins to a single plugin pack #1886
modules/performance: add an option to combine plugins to a single plugin pack #1886
Changes from all commits
3258347
388c522
e50320c
9129c9b
77ee33a
ad21cef
4792e62
8e8932a
d2a1e63
9a8f196
f8c7b7c
fa30bc6
435713d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Having to define this explicitly feels fragile... What would happen if we add another plugin module but don't notice we need to define addition paths to link?
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.
Additional paths will be missing from user runtime if
combinePlugins
is enabled, so it depends on what files are missing. For telescope here, it pretty much doesn't matter, because planets is a picker that is only for testing. Most plugins doesn't need any paths except standard.I think in a description of this options should be clear, that this option is experimental, doesn't give any guaranties etc. So if it works for user's config, he'll get a performance boost, if not — sorry. I've added EXPERIMENTAL to the description for that purpose. But English is not native for me, if you have better description, we can update it.
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 think the description is good. I think we can support this feature with less guarantees on breaking changes, i.e we won't add tests for all the plugins trying to enable the option, as this might be a too large maintenance burden, but we'll gladly include workarounds if people suggest them