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

[build] Removes commonjs transforms #66506

Merged
merged 6 commits into from
May 21, 2020
Merged

[build] Removes commonjs transforms #66506

merged 6 commits into from
May 21, 2020

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented May 14, 2020 •
edited
Loading

Blocker for tree shaking

It's not necessary for us to transform to commonjs, and removing the step reduces the builds by ~27%

# averages over three runs 227.27 (master) to 164.7
for i in {1..3}; do time node scripts/build_kibana_platform_plugins.js --no-cache --workers=6; done

@tylersmalley tylersmalley force-pushed the remove-commonjs-transform branch 14 times, most recently from b724a7a to 6c58e26 Compare May 20, 2020 12:56
@tylersmalley tylersmalley force-pushed the remove-commonjs-transform branch from 6c58e26 to 5b14d95 Compare May 20, 2020 19:41
tylersmalley
tylersmalley commented May 20, 2020
View reviewed changes
packages/kbn-interpreter/.babelrc
@@ -1,6 +1,7 @@
{
"presets": ["@kbn/babel-preset/webpack_preset"],
"plugins": [
"@babel/plugin-transform-modules-commonjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This project is required from both server/client code and only has a single build output - a follow-up could be to build separate packages but out of the scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yeah, it used to build separate outputs...

Copy link
Member

Choose a reason for hiding this comment

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

The plan is to remove this package eventually as the expressions plugin should already be exposing all of this functionality.

@tylersmalley tylersmalley added v7.8.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team labels May 20, 2020
@elasticmachine
Copy link
Contributor

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

@tylersmalley tylersmalley marked this pull request as ready for review May 20, 2020 19:56
@tylersmalley tylersmalley requested a review from a team May 20, 2020 19:56
@tylersmalley tylersmalley requested review from a team as code owners May 20, 2020 19:56
@tylersmalley tylersmalley mentioned this pull request May 20, 2020
Closed
2 tasks
Updates APM import to not import across boundaries
54273b7 
Introduced in elastic#66432 and I have a more thorough PR to prevent imports in
server from public in elastic#67149

Signed-off-by: Tyler Smalley <[email protected]>
@tylersmalley tylersmalley requested a review from a team as a code owner May 21, 2020 05:30
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label May 21, 2020
@elasticmachine
Copy link
Contributor

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

@tylersmalley
Copy link
Contributor Author

@elasticmachine merge upstream

majagrubic
majagrubic approved these changes May 21, 2020
View reviewed changes
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

kibana-app changes look good!

sorenlouv
sorenlouv approved these changes May 21, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv 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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@tylersmalley tylersmalley merged commit c675301 into elastic:master May 21, 2020
@tylersmalley tylersmalley mentioned this pull request May 21, 2020
Merged
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 66506 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 25, 2020
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request May 26, 2020
[build] Removes commonjs transforms (elastic#66506)
880779a 
Signed-off-by: Tyler Smalley <[email protected]>
Co-authored-by: spalger <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	packages/kbn-optimizer/src/integration_tests/basic_optimization.test.ts
#	src/optimize/base_optimizer.js
#	src/optimize/create_ui_exports_module.js
@tylersmalley tylersmalley mentioned this pull request May 26, 2020
Merged
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

6 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

spalger added a commit that referenced this pull request Jun 4, 2020
@spalger @elasticmachine
[7.x] [build] Removes commonjs transforms (#66506) (#67375)
22c539f 
Co-authored-by: spalger <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 4, 2020
tylersmalley pushed a commit that referenced this pull request Jun 10, 2020
[eslint] Prevents importing of public code into server (#67149)
8a5a7c3 
We should not be allowing importing of public into server. Any shared code should reside in a common directory. After #66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Blocks #66506

Signed-off-by: Tyler Smalley <[email protected]>
tylersmalley pushed a commit to tylersmalley/kibana that referenced this pull request Jun 10, 2020
[eslint] Prevents importing of public code into server (elastic#67149)
e1191d7 
We should not be allowing importing of public into server. Any shared code should reside in a common directory. After elastic#66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Blocks elastic#66506

Signed-off-by: Tyler Smalley <[email protected]>
tylersmalley pushed a commit that referenced this pull request Jun 10, 2020
@elasticmachine
[eslint] Prevents importing of public code into server (#67149) (#68782)
a8718ef 
We should not be allowing importing of public into server. Any shared code should reside in a common directory. After #66506, this will not even be possible as we will no longer be transpiling public code into commonjs.

Blocks #66506

Signed-off-by: Tyler Smalley <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@tylersmalley tylersmalley mentioned this pull request Jul 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

@spalger spalger spalger approved these changes

@lukeelmers lukeelmers lukeelmers approved these changes

@majagrubic majagrubic majagrubic approved these changes

Assignees
No one assigned
Labels
Feature:Build Tooling 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.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tylersmalley @elasticmachine @kibanamachine @sorenlouv @spalger @lukeelmers @majagrubic