Add Gulpfile.ts by weswigham · Pull Request #9068 · microsoft/TypeScript

weswigham · 2016-06-10T06:50:25Z

Last fall I always said I'd probably get around to doing this at some point.

I was frustrated earlier today by our Jake-based build, and I had always talked about how it'd be easier to work with as another task runner. So this evening I did just what I always said I would - I ported our Jakefile to a Gulpfile. Added bonus: It's now written in typescript! And there's a gulp help command!

Gulp is orders of magnitude more popular than Jake nowadays, and sees much more active use - external contributors are more likely to have existing experiences with gulp. We can also continue moving parts of the build to in-memory streams for a bit more efficiency once this is done.

@DanielRosenwasser @sandersn @yuit You guys should definitely take a look.

sandersn · 2016-06-10T14:58:17Z

@rbuckton can you take a look too? I think you know the most about gulp around here.

sandersn · 2016-06-10T15:31:58Z

I almost always C-c tests when they pass and start linting. And when I realise I'm building the wrong thing, etc. The rename trick probably isn't going to work. :(

sandersn · 2016-06-10T15:34:55Z

-var LKGDirectory = "lib/";
-
-var copyright = "CopyrightNotice.txt";
-var thirdParty = "ThirdPartyNoticeText.txt";


I don't see this (yet) in the gulpfile.

It was unused. I'll double check if its actually used in the Jakefile and that I forgot something.

I double checked now that I am not on the bus. It's unused.

yuit · 2016-06-10T17:18:14Z

talk with @sandersn instead of deleting the JakeFile completely, may be we should keep the JakeFile for now and travis-CI to use Jake. In the team, we will start using gulp internally for a week or so before switch. Also @sandersn suggests that we should do this on Mon/Tue. instead of Friday. if we decide to merge so things won't break on weekend 😸

rbuckton · 2016-06-10T17:19:34Z

Have you considered using the gulp-typescript plugin to replace compileFile and leverage our existing tsconfig.json files?

weswigham · 2016-06-10T20:55:24Z

@yuit Considering its build is still broken (workin' on it), I have no illusions about this is getting merged this weekend, lol. As for keeping around the Jakefile for awhile... 🤷. I guess we could.

@rbuckton Yes! This is a thing I want to do! The only reason I didn't was because I wanted to start to replicating our Jakefile's functionality as closely as possible. (Maybe we wanted to keep on using our command-line interface? I dunno)

weswigham · 2016-06-10T21:04:35Z

@sandersn I should have a fix incoming for the terribad workaround - passing an empty array to the types compiler option (thanks @RyanCavanaugh). Unfortunately, we don't (presently) support passing an empty array on the command line - so I'm going to make a PR for that. Then I'm going to swap us to using gulp-typescript in the gulpfile in this branch, which should also remove the issue.

sandersn · 2016-06-11T00:49:34Z

+    });
+});
+
+gulp.task("generate-code-coverage", "Generates code coverage data via instanbul", ["tests"], (done) => {


Typo: istanbul

sandersn · 2016-06-11T01:00:38Z

I vote that you undo the removal of the Jakefile and make sure Travis still works with jake, then merge this. We can test it over the next week or so and then switch Travis.

…nstead

weswigham · 2016-06-14T21:55:08Z

@sandersn @rbuckton @yuit Updated! Kept the Jakefile for a transition period, as requested (and the package.json pointing at jake), and rewrote all the compilation tasks in the gulpfile to use gulp-typescript which, conveniently, removed the need for the @types folder hackery. Another interesting change this required was that instead of just building tsc.js to use the built local compiler, the entire services layer has to get made instead (in order to make our node-compatible typescript.js).

rbuckton · 2016-06-17T22:14:54Z

+
+function exec(cmd: string, args: string[], complete: () => void = (() => {}), error: (e: any, status: number) => void = (() => {})) {
+    console.log(`${cmd} ${args.join(" ")}`);
+    const ex = cp.spawn(cmd, args);


consider:

const ex = cp.spawn(cmd, args, { stdio: "inherit" });

Then you can drop the ex.stdout.pipe(process.stdout); and ex.stderr.pipe(process.stderr); lines.

Also if you do:

const ex = cp.spawn(isWin ? "cmd" : "/bin/sh", [isWin ? "/c" : "-c", cmd, ...args], { stdio: "inherit", windowsVerbatimArguments: true });

It will use the OS shell to launch the command, letting spawn leverage the PATH environment variable and automatically handle executables without appending .cmd, etc.

weswigham · 2016-06-20T22:37:08Z

@rbuckton Updated to handle your comments and, now that newer is guarding every stream, add a watch task which runs runtests-parallel.

weswigham · 2016-06-20T23:01:23Z

Sourcemaps should now work in every context (browser, vscode); without using inline sources.

rbuckton · 2016-06-21T00:23:19Z

-    js.pipe(sourcemaps.write("."))
+
+    return merge2([
+        js.pipe(gIf(useDebugMode, insert.prepend(fs.readFileSync(copyright))))


Can we make this less eager using lazypipe?

rbuckton · 2016-06-21T21:26:22Z

        .pipe(sourcemaps.init())
        .pipe(tsc(localCompilerProject))
-        .pipe(gIf(useDebugMode, insert.prepend(fs.readFileSync(copyright))))
+        .pipe(gIf(useDebugMode, prependCopyright()))


I played around with this and this still ends up eagerly calling fs.readFileSync.

Another option that does not use gulp-if/lazypipe would be:

import { PassThrough } from "stream"; ... return localCompilerProject.src() .pipe(newer(builtLocalCompiler)) .pipe(sourcemaps.init()) .pipe(tsc(localCompilerProject)) .pipe(useDebugMode ? insert.prepend(fs.readFileSync(copyright)) : pass()) .pipe(sourcemaps.write(".")) .pipe(gulp.dest(builtLocalDirectory)); ... function pass() { return new PassThrough({ objectMode: true }); }

Or why not just insert.prepend(useDebugMode ? fs.readFileSync(copyright) : ""), any avoid both of those dependencies?

mhegazy · 2016-06-22T17:31:44Z

+});
+
+const scriptsTsdJson = path.join(scriptsDirectory, "tsd.json");
+gulp.task("tsd-scripts", `Runs \`tsd --config ${scriptsTsdJson}\` install`, [], (done) => {


i would remove this.

…configs

rbuckton · 2016-06-23T01:41:48Z

 const nodeServerInFile = "tests/webTestServer.ts";
 gulp.task(nodeServerOutFile, false, [servicesFile], () => {
-    const settings: tsc.Settings = getCompilerSettings({}, /*useBuiltCompiler*/ true);
+    const settings: tsc.Settings = getCompilerSettings({module: "commonjs"}, /*useBuiltCompiler*/ true);


nit: whitespace

I'm not sure what whitespace you're complaining about, but whatever it is I guess it should be codified because our lint rules don't catch it.

probably { module: "commonjs" } -- spaces after { and before}

Yeah - this style is inconsistently enforced, I have instances both with and without spaces in the compiler. We need a lint rule for this, one way or the other, if people are going to display a preference.

Additionally, is this supposed to be consistent with binding patterns and/or template strings? I'm internally consistent in the Gulpefile (no leading/trailing whitespace for any), but again, we're inconsistent within the compiler.

I can't find it, so probably we should just have a discussion and vote on which linter rule we want to turn on. (Does TSLint have it? ESLint does -- object curly spacing)

I don't see any tslint rules white would cover whitespace before/after curlys.

I've written a rule to check which is the dominant style - by and large with whitespace is more prevalent in our codebase, so I assume the default should be with whitespace.

I'm going to merge this without fixing these nits and the fix them when I merge the lint rule catching them, is that okay?

Works for me. @rbuckton ?

msftclas added the cla-already-signed label Jun 10, 2016

weswigham added the Infrastructure Issue relates to TypeScript team infrastructure label Jun 10, 2016

weswigham force-pushed the remove-jake branch from 458c2eb to 23727d7 Compare June 10, 2016 07:40

Add gulpfile with same tasks as Jakefile

2b1b58b

weswigham force-pushed the remove-jake branch from 23727d7 to 2b1b58b Compare June 10, 2016 08:50

sandersn reviewed Jun 10, 2016
View reviewed changes

weswigham added 3 commits June 10, 2016 10:44

Fix package.json casing

9dcaf2b

Quick fix for runtests-parallel

d756806

Merge branch 'master' into remove-jake

6c74545

sandersn reviewed Jun 11, 2016
View reviewed changes

weswigham added 7 commits June 14, 2016 12:39

Remove compileFile and exec browserify - use gulp-typescript instead

8f7c6e8

fixup! Remove compileFile and exec browserify - use gulp-typescript i…

52a1659

…nstead

Working on gulpifying things

e6838b6

Gulfile runs tests and compiles like a champ

1defdc3

Add back in Jakefile for transitional period

684ce6e

Add local patch for gulp-typescript types

2ff408f

Merge branch 'master' into remove-jake

2634a67

weswigham added 2 commits June 14, 2016 14:56

Fix typos in help text

f12d035

Give Jakefile its ending newline back

dc14ce2

rbuckton reviewed Jun 17, 2016
View reviewed changes

weswigham added 2 commits June 20, 2016 15:30

Use merge2, gulp-if, gulp-newer, and more projects

0e5b741

Add watch task

8ca1910

Working non-inline sourcemaps for runtests

9619e3a

browser tests now also loads sourcemaps from disk

9d10f7c

weswigham changed the title ~~Remove Jakefile.js, Add Gulpfile.ts~~ Add Gulpfile.ts Jun 20, 2016

Merge branch 'master' into remove-jake

b103857

rbuckton reviewed Jun 21, 2016
View reviewed changes

Lazypipes and better services stream management

d7970a5

rbuckton reviewed Jun 21, 2016
View reviewed changes

Do copyright without gulp-if and lazypipe

1c9ad5c

mhegazy reviewed Jun 22, 2016
View reviewed changes

weswigham added 2 commits June 22, 2016 10:55

Remove tsd scripts task from gulpfile

8aeb682

Make use of module compiler option explicit, add strip internal to ts…

d008da5

…configs

rbuckton reviewed Jun 23, 2016
View reviewed changes

Add Gulpfile lint to jake, fix lints

a2fdc7e

weswigham merged commit be6c34f into microsoft:master Jun 23, 2016

weswigham mentioned this pull request Jun 23, 2016

Object whitespace rule #9332

Merged

weswigham deleted the remove-jake branch October 12, 2017 08:05

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

weswigham commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Jun 10, 2016

Uh oh!

sandersn commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit commented Jun 10, 2016

Uh oh!

rbuckton commented Jun 10, 2016

Uh oh!

weswigham commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Jun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jun 11, 2016

Uh oh!

weswigham commented Jun 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Jun 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham commented Jun 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Jun 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Jun 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

weswigham commented Jun 10, 2016 •

edited

Loading

weswigham commented Jun 10, 2016 •

edited

Loading

weswigham commented Jun 14, 2016 •

edited

Loading

rbuckton Jun 17, 2016 •

edited

Loading

weswigham commented Jun 20, 2016 •

edited

Loading

weswigham commented Jun 20, 2016 •

edited

Loading

rbuckton Jun 21, 2016 •

edited

Loading