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

fix: don't compile right after generating a project #142

Merged
merged 2 commits into from
Mar 29, 2018

Conversation

jinwoo
Copy link
Member

@jinwoo jinwoo commented Mar 29, 2018

Compilation would fail because there's no source files.

Compilation would fail because there's no source files.
@jinwoo jinwoo requested a review from ofrobots March 29, 2018 17:09
@jinwoo jinwoo requested a review from a team March 29, 2018 17:09
@codecov-io
Copy link

codecov-io commented Mar 29, 2018

Codecov Report

Merging #142 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   96.64%   96.66%   +0.02%     
==========================================
  Files          10       10              
  Lines         328      330       +2     
  Branches       19       19              
==========================================
+ Hits          317      319       +2     
  Misses         11       11
Impacted Files Coverage Δ
test/test-kitchen.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30f461f...d41bf92. Read the comment docs.

cp.spawnSync('npm', ['install'], {stdio: 'inherit'});
// --ignore-scripts so that compilation doesn't happen because there's no
// source files yet.
cp.spawnSync('npm', ['install', '--ignore-scripts'], {stdio: 'inherit'});

Choose a reason for hiding this comment

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

If someone has a postinstall script, I don't think that will get invoked either. However, I expect this use case is probably not too common and, if needed, someone can always run npm install manually if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, most of the cases gts init is run from an empty directory. Even though someone had a postinstall script, they would know what they are doing and be able to figure things out.

@@ -113,6 +113,10 @@ test.serial('use as a non-locally installed module', async t => {
// server.ts has a lint error. Should error.
await t.throws(simpleExecp(`${GTS} check src/server.ts`, opts));

// Compilation shouldn't have happened. Hence no `build` directory.
const dirContents = fs.readdirSync(`${tmpDir.name}/kitchen`);
t.is(dirContents.indexOf('build'), -1);

Choose a reason for hiding this comment

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

Is it possible that npm run compile was run but had an error that caused the build directory to not be created?

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 verified that the build directory is created without my change.

Choose a reason for hiding this comment

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

Ok

@jinwoo
Copy link
Member Author

jinwoo commented Mar 29, 2018

I moved the new test to a better place.

@jinwoo jinwoo merged commit 32c412b into google:master Mar 29, 2018
@jinwoo jinwoo deleted the no-build-on-init branch March 29, 2018 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants