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

[APM] Make typescript optimization process compatible with NP #58984

Merged
merged 4 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,3 @@ package-lock.json
*.sublime-*
npm-debug.log*
.tern-project
x-pack/legacy/plugins/apm/tsconfig.json
apm.tsconfig.json
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/apm/dev_docs/typescript.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#### Optimizing TypeScript

Kibana and X-Pack are very large TypeScript projects, and it comes at a cost. Editor responsiveness is not great, and the CLI type check for X-Pack takes about a minute. To get faster feedback, we create a smaller APM TypeScript project that only type checks the APM project and the files it uses. This optimization consists of creating a `tsconfig.json` in APM that includes the Kibana/X-Pack typings, and editing the Kibana/X-Pack configurations to not include any files, or removing the configurations altogether. The script configures git to ignore any changes in these files, and has an undo script as well.
Kibana and X-Pack are very large TypeScript projects, and it comes at a cost. Editor responsiveness is not great, and the CLI type check for X-Pack takes about a minute. To get faster feedback, we create a smaller APM TypeScript project that only type checks the APM project and the files it uses. This optimization consists of modifying `tsconfig.json` in the X-Pack folder to only include APM files, and editing the Kibana configuration to not include any files. The script configures git to ignore any changes in these files, and has an undo script as well.

To run the optimization:

Expand Down
6 changes: 5 additions & 1 deletion x-pack/legacy/plugins/apm/scripts/optimize-tsconfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@

const { optimizeTsConfig } = require('./optimize-tsconfig/optimize');

optimizeTsConfig();
optimizeTsConfig().catch(err => {
// eslint-disable-next-line no-console
console.log(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do console.error here will ESLint not complain? You would want this on stderr anyway I presume.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, I'll move it to console.error (regardless of whether it will complain)

process.exit(1);
});
54 changes: 28 additions & 26 deletions x-pack/legacy/plugins/apm/scripts/optimize-tsconfig/optimize.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,26 @@
/* eslint-disable import/no-extraneous-dependencies */

const fs = require('fs');
const promisify = require('util').promisify;
const { promisify } = require('util');
const path = require('path');
const json5 = require('json5');
const execa = require('execa');

const copyFile = promisify(fs.copyFile);
const rename = promisify(fs.rename);
const readFile = promisify(fs.readFile);
const writeFile = promisify(fs.writeFile);

const {
xpackRoot,
kibanaRoot,
apmRoot,
tsconfigTpl,
filesToIgnore
} = require('./paths');
const { unoptimizeTsConfig } = require('./unoptimize');

function updateParentTsConfigs() {
function prepareParentTsConfigs() {
return Promise.all(
[
path.resolve(xpackRoot, 'apm.tsconfig.json'),
path.resolve(xpackRoot, 'tsconfig.json'),
path.resolve(kibanaRoot, 'tsconfig.json')
].map(async filename => {
const config = json5.parse(await readFile(filename, 'utf-8'));
Expand All @@ -50,32 +47,37 @@ function updateParentTsConfigs() {
);
}

async function addApmFilesToXpackTsConfig() {
const template = json5.parse(await readFile(tsconfigTpl, 'utf-8'));
const xpackTsConfig = path.join(xpackRoot, 'tsconfig.json');
const config = json5.parse(await readFile(xpackTsConfig, 'utf-8'));

await writeFile(
xpackTsConfig,
JSON.stringify({ ...config, ...template }, null, 2),
{ encoding: 'utf-8' }
);
}

async function setIgnoreChanges() {
for (const filename of filesToIgnore) {
await execa('git', ['update-index', '--skip-worktree', filename]);
}
}

const optimizeTsConfig = () => {
return unoptimizeTsConfig()
.then(() =>
Promise.all([
copyFile(tsconfigTpl, path.resolve(apmRoot, './tsconfig.json')),
rename(
path.resolve(xpackRoot, 'tsconfig.json'),
path.resolve(xpackRoot, 'apm.tsconfig.json')
)
])
)
.then(() => updateParentTsConfigs())
.then(() => setIgnoreChanges())
.then(() => {
// eslint-disable-next-line no-console
console.log(
'Created an optimized tsconfig.json for APM. To undo these changes, run `./scripts/unoptimize-tsconfig.js`'
);
});
};
async function optimizeTsConfig() {
await unoptimizeTsConfig();

await prepareParentTsConfigs();

await addApmFilesToXpackTsConfig();

await setIgnoreChanges();
// eslint-disable-next-line no-console
console.log(
'Created an optimized tsconfig.json for APM. To undo these changes, run `./scripts/unoptimize-tsconfig.js`'
);
}

module.exports = {
optimizeTsConfig
Expand Down
4 changes: 1 addition & 3 deletions x-pack/legacy/plugins/apm/scripts/optimize-tsconfig/paths.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
*/
const path = require('path');

const apmRoot = path.resolve(__dirname, '../..');
const xpackRoot = path.resolve(apmRoot, '../../..');
const xpackRoot = path.resolve(__dirname, '../../../../..');
const kibanaRoot = path.resolve(xpackRoot, '..');

const tsconfigTpl = path.resolve(__dirname, './tsconfig.json');
Expand All @@ -17,7 +16,6 @@ const filesToIgnore = [
];

module.exports = {
apmRoot,
xpackRoot,
kibanaRoot,
tsconfigTpl,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
{
"extends": "../../../apm.tsconfig.json",
"include": [
"./**/*",
"../../../plugins/apm/**/*",
"../../../typings/**/*"
"./plugins/apm/**/*",
"./legacy/plugins/apm/**/*",
"./typings/**/*"
],
"exclude": [
"**/__fixtures__/**/*",
"./e2e/cypress/**/*"
"./legacy/plugins/apm/e2e/cypress/**/*"
]
}
21 changes: 5 additions & 16 deletions x-pack/legacy/plugins/apm/scripts/optimize-tsconfig/unoptimize.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,21 @@
*/
/* eslint-disable import/no-extraneous-dependencies */

const path = require('path');
const execa = require('execa');
const fs = require('fs');
const promisify = require('util').promisify;
const removeFile = promisify(fs.unlink);
const exists = promisify(fs.exists);

const { apmRoot, filesToIgnore } = require('./paths');
const { filesToIgnore } = require('./paths');

async function unoptimizeTsConfig() {
for (const filename of filesToIgnore) {
await execa('git', ['update-index', '--no-skip-worktree', filename]);
await execa('git', ['checkout', filename]);
}

const apmTsConfig = path.join(apmRoot, 'tsconfig.json');
if (await exists(apmTsConfig)) {
await removeFile(apmTsConfig);
}
}

module.exports = {
unoptimizeTsConfig: () => {
return unoptimizeTsConfig().then(() => {
// eslint-disable-next-line no-console
console.log('Removed APM TypeScript optimizations');
});
unoptimizeTsConfig: async () => {
await unoptimizeTsConfig();
// eslint-disable-next-line no-console
console.log('Removed APM TypeScript optimizations');
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go to stderr too.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not an error though, am I misunderstanding what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

console.error logs to stderr instead of stdout. stdout is meant for output that can be piped around, and stderr for messages like this.

https://www.jstorimer.com/blogs/workingwithcode/7766119-when-to-use-stderr-instead-of-stdout

it's probably a controversial point though against "console.error is for errors" so either way I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got that, but you might have commented in the wrong file? Did you meant to comment on scripts/unoptimize-tsconfig.js? (I've changed it to console.error there as well).

}
};
6 changes: 5 additions & 1 deletion x-pack/legacy/plugins/apm/scripts/unoptimize-tsconfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@

const { unoptimizeTsConfig } = require('./optimize-tsconfig/unoptimize');

unoptimizeTsConfig();
unoptimizeTsConfig().catch(err => {
// eslint-disable-next-line no-console
console.log(err);
process.exit(1);
});
3 changes: 0 additions & 3 deletions x-pack/plugins/apm/tsconfig.json

This file was deleted.