Skip to content

fix: use local gts path for generated tsconfig.json #114

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 11, 2018
Merged

fix: use local gts path for generated tsconfig.json #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f54dae
fix: use local gts path for generated tsconfig.json
jinwoo Jan 11, 2018
f1b1ad2
move test to better place
jinwoo Jan 11, 2018
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
7 changes: 1 addition & 6 deletions src/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,8 @@ async function generateTsConfig(options: Options): Promise<void> {
}
}

// typescript expects relative paths to start with './'.
const baseConfig = './' +
path.relative(
options.targetRootDir,
path.resolve(options.gtsRootDir, 'tsconfig-google.json'));
const tsconfig = formatJson({
extends: baseConfig,
extends: './node_modules/gts/tsconfig-google.json',
Copy link

@DominicKramer DominicKramer Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should path.join be used instead of using / directly?

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 don't think we should have a different value here depending on on which platform the source code is generated, right?

Copy link

Choose a reason for hiding this comment

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

I'm not sure. I would expect tsconfig to be like package.json where we can always use /, but we should probably make sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Windows has always supported / as a path separator as well as \. So I don't see any issues with this if we consider all the current major OS's.

Copy link

Choose a reason for hiding this comment

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

Ok sounds good.

compilerOptions: {rootDir: '.', outDir: 'build'},
include: ['src/*.ts', 'src/**/*.ts', 'test/*.ts', 'test/**/*.ts'],
exclude: ['node_modules']
Expand Down
11 changes: 8 additions & 3 deletions test/test-kitchen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,15 @@ test.serial('use as a non-locally installed module', async t => {
const GTS = `${stagingPath}/kitchen/node_modules/.bin/gts`;
const tmpDir = tmp.dirSync({keep, unsafeCleanup: true});
await ncpp('test/fixtures', `${tmpDir.name}/`);

const opts = {cwd: `${tmpDir.name}/kitchen`};
// It's important to use `-n` here because we don't want to overwrite
// the version of gts installed, as it will trigger the npm install.
await simpleExecp(`${GTS} init -n`, opts);
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 changed -n to -y because a local gts must be installed so that the gts check below can find the tsconfig-google.json file.

await simpleExecp(`${GTS} init -y`, opts);
// The `extends` field must use the local gts path.
const tsconfigJson =
Copy link

Choose a reason for hiding this comment

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

nit: Could you use pify to promisify readFile so that you can await the readFile? Also, should you be using path.join here and in the t.deepEqual line?

Copy link
Member Author

Choose a reason for hiding this comment

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

This test file uses lots of ...Sync functions. I agree that using async functions is better but since this is a test file, I think we'd better make the code more consistent. WDYT?

Copy link

Choose a reason for hiding this comment

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

I agree its not that big of a deal since its a test file, especially if there are a lot of places where *Sync functions are used. If we wanted to make them async, it would probably be best as a follow-up PR.

fs.readFileSync(`${tmpDir.name}/kitchen/tsconfig.json`, 'utf8');
const tsconfig = JSON.parse(tsconfigJson);
t.deepEqual(tsconfig.extends, './node_modules/gts/tsconfig-google.json');

await simpleExecp(`${GTS} check kitchen/src/server.ts`, opts);
if (!keep) {
tmpDir.removeCallback();
Expand Down