Skip to content

Commit

Permalink
[typescript] continue to use the default config in development (#21966)
Browse files Browse the repository at this point in the history
* [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
  • Loading branch information
Spencer authored Aug 14, 2018
1 parent cd433cc commit 5db7245
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 49 deletions.
30 changes: 24 additions & 6 deletions src/dev/build/tasks/transpile_typescript_task.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,36 @@
* under the License.
*/

import { exec } from '../lib';
import { exec, write } from '../lib';
import { Project } from '../../typescript';

export const TranspileTypescriptTask = {
description: 'Transpiling sources with typescript compiler',

async run(config, log, build) {
const tsConfigPaths = [
build.resolvePath('tsconfig.json'),
build.resolvePath('tsconfig.browser.json')
];
const defaultProject = new Project(build.resolvePath('tsconfig.json'));
const browserProject = new Project(build.resolvePath('tsconfig.browser.json'));

for (const tsConfigPath of tsConfigPaths) {
// update the default config to exclude **/public/**/* files
await write(defaultProject.tsConfigPath, JSON.stringify({
...defaultProject.config,
exclude: [
...defaultProject.config.exclude,
'src/**/public/**/*'
]
}));

// update the browser config file to include **/public/**/* files
await write(browserProject.tsConfigPath, JSON.stringify({
...browserProject.config,
include: [
...browserProject.config.include,
'src/**/public/**/*'
]
}));

// compile each typescript config file
for (const tsConfigPath of [defaultProject.tsConfigPath, browserProject.tsConfigPath]) {
log.info(`Compiling`, tsConfigPath, 'project');
await exec(
log,
Expand Down
19 changes: 12 additions & 7 deletions src/dev/typescript/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,7 @@ function parseTsConfig(path: string) {
throw error;
}

const files: string[] | undefined = config.files;
const include: string[] | undefined = config.include;
const exclude: string[] | undefined = config.exclude;
return { files, include, exclude };
return config;
}

function testMatchers(matchers: IMinimatch[], path: string) {
Expand All @@ -54,11 +51,19 @@ function testMatchers(matchers: IMinimatch[], path: string) {
export class Project {
public directory: string;
public name: string;
private include: IMinimatch[];
private exclude: IMinimatch[];
public config: any;

private readonly include: IMinimatch[];
private readonly exclude: IMinimatch[];

constructor(public tsConfigPath: string, name?: string) {
const { files, include, exclude = [] } = parseTsConfig(tsConfigPath);
this.config = parseTsConfig(tsConfigPath);

const { files, include, exclude = [] } = this.config as {
files?: string[];
include?: string[];
exclude?: string[];
};

if (files || !include) {
throw new Error(
Expand Down
1 change: 0 additions & 1 deletion src/dev/typescript/projects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import { Project } from './project';

export const PROJECTS = [
new Project(resolve(REPO_ROOT, 'tsconfig.json')),
new Project(resolve(REPO_ROOT, 'tsconfig.browser.json'), 'kibana (browser)'),
new Project(resolve(REPO_ROOT, 'x-pack/tsconfig.json')),

// NOTE: using glob.sync rather than glob-all or globby
Expand Down
73 changes: 40 additions & 33 deletions src/optimize/base_optimizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,42 +295,49 @@ export default class BaseOptimizer {
};

// when running from source transpile TypeScript automatically
const isSourceConfig = {
module: {
rules: [
{
resource: createSourceFileResourceSelector(/\.tsx?$/),
use: maybeAddCacheLoader('typescript', [
{
loader: 'ts-loader',
options: {
transpileOnly: true,
experimentalWatchApi: true,
onlyCompileBundledFiles: true,
configFile: fromRoot('tsconfig.browser.json'),
compilerOptions: {
sourceMap: Boolean(this.sourceMaps),
const getSourceConfig = () => {
// dev/typescript is deleted from the distributable, so only require it if we actually need the source config
const { Project } = require('../dev/typescript');
const browserProject = new Project(fromRoot('tsconfig.browser.json'));

return {
module: {
rules: [
{
resource: createSourceFileResourceSelector(/\.tsx?$/),
use: maybeAddCacheLoader('typescript', [
{
loader: 'ts-loader',
options: {
transpileOnly: true,
experimentalWatchApi: true,
onlyCompileBundledFiles: true,
configFile: fromRoot('tsconfig.json'),
compilerOptions: {
...browserProject.config.compilerOptions,
sourceMap: Boolean(this.sourceMaps),
}
}
}
}
]),
}
]
},
]),
}
]
},

stats: {
// when typescript doesn't do a full type check, as we have the ts-loader
// configured here, it does not have enough information to determine
// whether an imported name is a type or not, so when the name is then
// exported, typescript has no choice but to emit the export. Fortunately,
// the extraneous export should not be harmful, so we just suppress these warnings
// https://github.com/TypeStrong/ts-loader#transpileonly-boolean-defaultfalse
warningsFilter: /export .* was not found in/
},
stats: {
// when typescript doesn't do a full type check, as we have the ts-loader
// configured here, it does not have enough information to determine
// whether an imported name is a type or not, so when the name is then
// exported, typescript has no choice but to emit the export. Fortunately,
// the extraneous export should not be harmful, so we just suppress these warnings
// https://github.com/TypeStrong/ts-loader#transpileonly-boolean-defaultfalse
warningsFilter: /export .* was not found in/
},

resolve: {
extensions: ['.ts', '.tsx'],
},
resolve: {
extensions: ['.ts', '.tsx'],
},
};
};

// We need to add react-addons (and a few other bits) for enzyme to work.
Expand Down Expand Up @@ -407,7 +414,7 @@ export default class BaseOptimizer {
commonConfig,
IS_KIBANA_DISTRIBUTABLE
? isDistributableConfig
: isSourceConfig,
: getSourceConfig(),
this.uiBundles.isDevMode()
? webpackMerge(watchingConfig, supportEnzymeConfig)
: productionConfig
Expand Down
5 changes: 4 additions & 1 deletion tsconfig.browser.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
"module": "esnext",
},
"include": [
"src/**/public/**/*"
// in the build we populate this to include **/public/**/* but
// if we did that when running from source IDEs won't properly map
// public files to this config file and instead just use the defaults, ie. "strict": false,
// **/public/**/*
],
"exclude": [
"src/**/__fixtures__/**/*"
Expand Down
6 changes: 5 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@
],
"exclude": [
"src/**/__fixtures__/**/*",
"src/**/public/**/*"
// In the build we actually exclude **/public/**/* from this config so that
// we can run the TSC on both this and the .browser version of this config
// file, but if we did it during development IDEs would not be able to find
// the tsconfig.json file for public files correctly.
// "src/**/public/**/*"
]
}

0 comments on commit 5db7245

Please sign in to comment.