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

[tsc] Ensure shared TypeScript modules are compiled to be compatible with Node and Webpack #32578

Merged
merged 1 commit into from
Mar 7, 2019

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Mar 6, 2019

Summary

This modifies the TypeScript build step order to ensure that any shared code is built to a format that both Node and Webpack can use. I did this by making the browser build complete before the server build so that shared code is built into CommonJS modules.

This "works" with the current modules that are shared since the shared modules will be in CommonJS format (compiled by the serve build) and we're actually only import type information right now across to the client-side code.

I think this solution is fine for the time being, because it is temporary. We plan to move to immutable bundles with no JS or TS compilation happening at all in production builds. Other options before we get to immutable bundles:

  • Ship TS source files and the TypeScript compiler with ts-loader to be able to generate ES6 Modules when re-optimizing in production.
  • After upgrading to Babel 7 with TS, compile browser babel output to .browser.js and prefer that when importing via Webpack.
  • Use a static file system for all of the webpack source files, server files, or both, which would allow us to potentially have separate source files for browser and server use.

From #32567

The core issue here is that for production builds, we build TS code for the server (tsconfig.json) as commonjs modules and for the browser (tsconfig.browser.json) as esmodules. If a module is imported by both the server and client-side builds, then the client-side build wins and the file is outputted as an esmodule, which Node cannot read.

In order to support TS code sharing (or even just type sharing) between server and browser code, we need to be outputting to a format that both Node and Webpack can read. In my testing it seems that outputting to CommonJS modules works for both environments.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations

@joshdover joshdover added the build label Mar 6, 2019
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

The reason we need to use esm for the browser typescript is so we have esm features like mutable export bindings, but if you find a way around that I'd be happy to have this.

@joshdover joshdover changed the title [tsc] Use same module output for browser as server TS builds [tsc] Ensure shared TypeScript modules are compiled to CommonJS Mar 6, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover
Copy link
Contributor Author

@spalger That's what I ended up realizing. I've modified this now to make it build browser TS first, and then server TS. This "works" with the current modules that are shared since the shared modules will be in CommonJS format (compiled by the serve build) and we're actually only import type information right now across to the client-side code.

I think this solution is fine for the time being, because it is temporary. We plan to move to immutable bundles with no JS or TS compilation happening at all in production builds. If we wanted to a slightly better intermediate step, we could ship TS source files and the TypeScript compiler with ts-loader to be able to generate ES6 Modules when re-optimizing in production.

@joshdover joshdover changed the title [tsc] Ensure shared TypeScript modules are compiled to CommonJS [tsc] Ensure shared TypeScript modules are compiled to be compatible with Node and Webpack Mar 7, 2019
@spalger
Copy link
Contributor

spalger commented Mar 7, 2019

Yeah, that sounds reasonable to me. Maybe once we have babel7 we could compile these files to .browser.js or something and have webpack pickup those versions over the .js files, so we would still be able to avoid shipping TypeScript, avoid duplicating the file tree, and be able to avoid problems importing ui code from the server when necessary. Alternatively, we can move forward with a static file system for all of the webpack source files, server files, or both, which would allow us to potentially have separate source files for browser and server use.

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

Can you update the comment before merging with details about the drawback to this that you and @spalger discussed in the PR comments? Right now it just explains why we do it but not what the consequence of this is, even if it is only temporary.

@joshdover joshdover merged commit c03e9a0 into elastic:master Mar 7, 2019
@joshdover joshdover deleted the ts-commonjs branch March 7, 2019 15:26
joshdover added a commit to joshdover/kibana that referenced this pull request Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Team:Operations Team label for Operations Team v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants