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

[build/ts] transpile public code with webpack-specific ts config #21865

Merged
merged 4 commits into from
Aug 10, 2018

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Aug 9, 2018

Right now the build process is running TypeScript code through a single transpilation process which produces the JS code that ships with the distributable. This process causes problems because when running the optimizer from source we use a slightly different config

target: 'es5',
module: 'esnext',
, which instructs typescript to build modules using ESM, which webpack understands, but in the build we transpile this TypeScript to commonjs, which does not support features that are important to the strategy we are using to maintain BWC with the legacy platform as we migrate to the new platform.

This implements a tsconfig.browser.json file at the root of the repository that extends the default tsconfig.json file and includes a src/**/public/**/* code, which the root tsconfig.json file now excludes. This new config file is added to the list of projects that we lint, is shared with webpack, and is built along with the default project in the build process to create JS code that uses esm for webpack to consume.

@spalger spalger added review Team:Operations Team label for Operations Team v7.0.0 v6.5.0 labels Aug 9, 2018
@spalger spalger requested review from jbudz and tylersmalley August 9, 2018 21:57
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@jbudz

This comment has been minimized.

return {
...jestConfig,
globals: {
...(jestConfig.globals || {}),
'ts-jest': {
skipBabel: true,
tsConfigFile: getTsProjectForAbsolutePath(filePath).tsConfigPath,
tsConfigFile,
Copy link
Member

Choose a reason for hiding this comment

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

for whatever reason I thought we were using the browser env. any ideas if jest still includes jsdom even in node?

in any case that might explain https://github.com/elastic/kibana/pull/21811/files. I had to manually include the fetch mock browser client because it kept using node-fetch instead of window.fetch. should we not be setting anything on window there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we have jest configured to include jsdom, but that only mocks out some things, mostly the document/DOM related stuff. Things like fetch() are not supported. This change is specifically necessary because our browser based config is assuming that the code will be run through webpack before being executed by a browser, which would transpile ESM into webpack requires, but when running the code in jest it is run in node right out of TypeScript, so we need to use the node config to get commonJS

Copy link
Contributor Author

@spalger spalger Aug 10, 2018

Choose a reason for hiding this comment

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

An alternative option would be to use a light webpack-like transform on these files after then are put through typescript with our browser-based config, but I don't see any reason for the additional work that would take.

@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger spalger merged commit 06b1af3 into elastic:master Aug 10, 2018
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 10, 2018
…stic#21865)

Right now the build process is running TypeScript code through a single transpilation process which produces the JS code that ships with the distributable. This process causes problems because when running the optimizer from source we use a slightly different config https://github.com/elastic/kibana/blob/33c6ade75631fb3f56816bc134dff83d08f4f387/src/optimize/base_optimizer.js#L312-L313, which instructs typescript to build modules using ESM, which webpack understands, but in the build we transpile this TypeScript to commonjs, which does not support features that are important to the strategy we are using to maintain BWC with the legacy platform as we migrate to the new platform.

This implements a `tsconfig.browser.json` file at the root of the repository that extends the default `tsconfig.json` file and includes a `src/**/public/**/*` code, which the root `tsconfig.json` file now excludes. This new config file is added to the list of projects that we lint, is shared with webpack, and is built along with the default project in the build process to create JS code that uses esm for webpack to consume.
spalger pushed a commit that referenced this pull request Aug 11, 2018
#21865) (#21893)

Backports the following commits to 6.x:
 - [build/ts] transpile public code with webpack-specific ts config  (#21865)
@spalger
Copy link
Contributor Author

spalger commented Aug 11, 2018

6.5/6.x: 835ed85

@spalger spalger deleted the fix/build/webpack-specific-tsc branch August 11, 2018 01:25
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 14, 2018
In elastic#21865 we tried to make a separate config file that would be used for
browser TS files by excluding them from the default config file and
adding a second that included them. This works fine in the build, but
IDE integrations rely on being able to automatically discover a
`tsconfig.json` file in a parent directory of the file being edited,
which doesn't work when the tsconfig.json file is found, but excludes
the file being edited. In this situation IDE integrations silently
fallback to the default TSConfig settings, which don't show the types of
compiler errors that will become issues in CI, like implicit any
warnings.

This implements the original strategy we tried in elastic#21865 of mutating the
tsconfig.json file before we run the tsc in the build so that the config
files select the right files at build time.
spalger pushed a commit that referenced this pull request Aug 14, 2018
* [typescript] continue to use the default config in development

In #21865 we tried to make a separate config file that would be used for
browser TS files by excluding them from the default config file and
adding a second that included them. This works fine in the build, but
IDE integrations rely on being able to automatically discover a
`tsconfig.json` file in a parent directory of the file being edited,
which doesn't work when the tsconfig.json file is found, but excludes
the file being edited. In this situation IDE integrations silently
fallback to the default TSConfig settings, which don't show the types of
compiler errors that will become issues in CI, like implicit any
warnings.

This implements the original strategy we tried in #21865 of mutating the
tsconfig.json file before we run the tsc in the build so that the config
files select the right files at build time.

* [tslint] remove browser config from default projects list since it is empty

* [build/ts] fix typo
spalger pushed a commit to spalger/kibana that referenced this pull request Aug 14, 2018
…ic#21966)

* [typescript] continue to use the default config in development

In elastic#21865 we tried to make a separate config file that would be used for
browser TS files by excluding them from the default config file and
adding a second that included them. This works fine in the build, but
IDE integrations rely on being able to automatically discover a
`tsconfig.json` file in a parent directory of the file being edited,
which doesn't work when the tsconfig.json file is found, but excludes
the file being edited. In this situation IDE integrations silently
fallback to the default TSConfig settings, which don't show the types of
compiler errors that will become issues in CI, like implicit any
warnings.

This implements the original strategy we tried in elastic#21865 of mutating the
tsconfig.json file before we run the tsc in the build so that the config
files select the right files at build time.

* [tslint] remove browser config from default projects list since it is empty

* [build/ts] fix typo
spalger pushed a commit that referenced this pull request Aug 15, 2018
… (#21973)

* [typescript] continue to use the default config in development

In #21865 we tried to make a separate config file that would be used for
browser TS files by excluding them from the default config file and
adding a second that included them. This works fine in the build, but
IDE integrations rely on being able to automatically discover a
`tsconfig.json` file in a parent directory of the file being edited,
which doesn't work when the tsconfig.json file is found, but excludes
the file being edited. In this situation IDE integrations silently
fallback to the default TSConfig settings, which don't show the types of
compiler errors that will become issues in CI, like implicit any
warnings.

This implements the original strategy we tried in #21865 of mutating the
tsconfig.json file before we run the tsc in the build so that the config
files select the right files at build time.

* [tslint] remove browser config from default projects list since it is empty

* [build/ts] fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Operations Team label for Operations Team v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants