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

Attempt to reduce Core bundle size with type exports. #92221

Merged
merged 17 commits into from
Mar 3, 2021

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Feb 22, 2021

Summary

Closes #58113
When a developer exports something from a module that can be at the same time runtime code (for example, a Class) and a type, Babel doesn't remove such export.
It can affect the Page load bundle size, since often we want to provide a type to 3rd party modules, but not an implementation.
In this PR I leverage isolatedModules flag for tsc compiler that runs compilation for every file independently. To make it possible, tsc enforces all the types to be re-exported as export type { ... }. This, in turn, requires developers to evaluate every re-export to decide how it should be exported.
The algorithm is simple:

  • I want to export something as a type / an interface: export type {... }
  • I want to export something as runtime code: export { ... }

By default, always export classes as export type if you don't want users to rely on a concrete implementation.
This heuristic with manual export type declaration helps our build chain to strip unnecessary runtime exports.
In this PR, it shaved off about 10% of the page load bundle size of Core.
I already leveraged this approach once in #79635, which saved us ~220 KB there.

@mshustov mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Feb 22, 2021
@mshustov mshustov marked this pull request as ready for review February 25, 2021 11:51
@mshustov mshustov requested review from a team as code owners February 25, 2021 11:51
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Changes to src/core/server/csp/index.ts LGTM

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in src/core/server/csp/index.ts LGTM.

@@ -9,4 +9,5 @@
import { CspConfig, ICspConfig } from './csp_config';
Copy link
Member

Choose a reason for hiding this comment

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

question: out of curiosity, is there any benefits in doing import + export comparing to only using export's like this or is it just a matter of personal taste?

export type { ICspConfig } from './csp_config';
export type { CspConfigType } from './config';

export { config } from './config';
export { CspConfig } from './csp_config';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

out of curiosity, is there any benefits in doing import + export comparing to only using export's like this or is it just a matter of personal taste?

I don't think it provides extra benefits. The compiler already knows how the imported code is used in the file. So it can remove all the imports used as interfaces or types.

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

@kbn/test changes LGTM

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

I wish TS would provide an export { Class, type Foo } from x syntax at some point though.

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM! Just added 1 NIT comment. Feel free to dismiss it if it doesn't make sense.

src/core/server/elasticsearch/legacy/index.ts Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 447 431 -16

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 480.1KB 432.1KB -48.0KB

History

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

@mshustov mshustov merged commit f5e8b10 into elastic:master Mar 3, 2021
@mshustov mshustov deleted the core-ts-isolatemodules branch March 3, 2021 16:21
mshustov added a commit that referenced this pull request Mar 3, 2021
* compile core files as isolated modules

* fix export problems for isolated modules

* apply changes to kbn-test as core imports from it

* fix some exports

* fix lint errors

* update new exports

* fix eslint error

* expand export * where it is possible

* update docs

* update docs

* fix eslint error
# Conflicts:
#	api_docs/core.json
#	api_docs/core_application.json
#	api_docs/core_http.json
#	api_docs/core_saved_objects.json
#	src/core/public/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported chore release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor type exports to type only exports
8 participants