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.

Sign up for GitHub

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

Jump to bottom

[remove] production deps which are only used in public code #75838

Merged
merged 4 commits into from
Aug 25, 2020
Merged

[remove] production deps which are only used in public code #75838

merged 4 commits into from
Aug 25, 2020

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Aug 25, 2020 •
edited
Loading

With #73591 we are now building Kibana Platform plugins from source when building them, which means we can move all the dependencies which are only used by code in public directories from dependencies to devDependencies. To determine which dependencies are eligible I wrote a script that I'm not planning to merge just yet but will likely get up in a PR shortly. This tool will allow us to enforce this going forward.

For now, we should be able to drop a bunch of files from the build. When submitting the stats are pretty promising, the total file count of the distributable was reduced by 14,050 files, from 80,896 to 66,846.

@spalger spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.10.0 v8.0.0 labels Aug 25, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@spalger spalger marked this pull request as ready for review August 25, 2020 06:12
@spalger spalger requested review from a team as code owners August 25, 2020 06:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

dgieselaar
dgieselaar approved these changes Aug 25, 2020
View reviewed changes
Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

APM changes LGTM.

@spalger: do the package.jsons edited for APM impact the files that end up in the build, or was it just a consequence of running the script?

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 25, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

tylersmalley
tylersmalley reviewed Aug 25, 2020
View reviewed changes
x-pack/plugins/security_solution/yarn.lock
@@ -1 +0,0 @@
../../../yarn.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this was able to be removed since there are no longer any non-dev dependencies for it's package.json.

@spalger
Copy link
Contributor Author

spalger commented Aug 25, 2020

@spalger: do the package.jsons edited for APM impact the files that end up in the build, or was it just a consequence of running the script?

They impact the packages that are installed in node_modules within the final distributable, but they don't impact the modules which are built into the bundles produced by the @kbn/optimizer, which is why I'm moving packages that are only used in the bundles.

@dgieselaar
Copy link
Member

@spalger alright, fwiw & as you probably know, the other packages in those files are only used for one-off scripts (that we run locally) and E2E tests. Should those not be moved to devDependencies as well?

@spalger
Copy link
Contributor Author

spalger commented Aug 25, 2020

the other packages in those files are only used for one-off scripts (that we run locally) and E2E tests. Should those not be moved to devDependencies as well?

I think it makes sense to do that, would you mind submitting a follow up PR?

@spalger spalger merged commit 9cafade into elastic:master Aug 25, 2020
@spalger spalger deleted the remove/unneeded-production-deps branch August 25, 2020 15:27
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 25, 2020
@spalger
[remove] production deps which are only used in public code (elastic#…
c834ee6 
…75838)

Co-authored-by: spalger <spalger@users.noreply.github.com>
# Conflicts:
#	package.json
#	x-pack/package.json
@spalger spalger mentioned this pull request Aug 25, 2020
Merged
spalger added a commit that referenced this pull request Aug 25, 2020
@spalger
[7.x] [remove] production deps which are only used in public code (#7…
2d26c41 
…5838) (#75893)

Co-authored-by: spalger <spalger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgieselaar dgieselaar dgieselaar approved these changes

@XavierM XavierM XavierM approved these changes

@tylersmalley tylersmalley tylersmalley approved these changes

@oatkiller oatkiller oatkiller approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Operations Team label for Operations Team v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@spalger @kibanamachine @elasticmachine @dgieselaar @oatkiller @tylersmalley @XavierM