Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jan 9, 2017

Fixes #13335 and #13352

@mhegazy
Copy link
Contributor

mhegazy commented Jan 9, 2017

@ivogabe any ideas what went wrong with the latest?

@ivogabe
Copy link
Contributor

ivogabe commented Jan 9, 2017

I thought that it only contained changes related to sourcemaps, but I'll take a look at it.

@ghost ghost merged commit 1040247 into master Jan 9, 2017
@ghost ghost deleted the gulp_typescript_version branch January 9, 2017 18:10
@ivogabe
Copy link
Contributor

ivogabe commented Jan 9, 2017

I think that the old behaviour was wrong and the current behaviour is correct. When a tsconfig file is placed in a subdirectory, gulp-typescript would emit incorrect paths, see ivogabe/gulp-typescript#461. Though this old behaviour seemed to work here.

Here's what happens:
src/compiler/tsconfig.json sets "outFile": "../../built/local/tsc.js". In the gulpfile, localCompilerProject.src() sets the base path of the source files to root/src/compiler. The generated file should then have root/src/compiler as base path too, and ../../built/local/tsc.js (value of outFile) as relative path. gulp.dest(".") then causes that this file is written as ../../built/local/tsc.js (thus, outside of the project directory).

Previously, the base path would be wrongly set to "" (yes, an empty string), which caused that the file would we written as built/local/tsc.js.

This can be fixed by using gulp.dest("src/compiler") (the location of the tsconfig file, such that the outFile is evaluated relative to this location), setting outFile: "built/local/tsc.js" (this path is relative to project root, so gulp.dest(".") works fine), or setting outFile: "tsc.js" and gulp.dest("built/local").

@ghost ghost mentioned this pull request Feb 13, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants