-
Notifications
You must be signed in to change notification settings - Fork 99
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
Implement fast incremental compiler #85
Changes from 35 commits
62a81de
63d5e1e
e43d55b
b8248d9
0362674
1db94d5
0955fc3
18774d7
39a53d3
3da13f9
64484df
2a2952b
5acf76c
069f3e5
87f5054
9f7da6b
9c64109
b03e1c8
8339612
53012fc
f4316dd
2feb95a
1327866
d5147d2
59ab4fd
8c42b49
624b70c
77f13e5
3f722d9
da18bba
54edcdc
8fad31c
0e4b8b5
fe5035e
a666892
0b4b4fe
59e2c77
ea8a494
b70d390
bdde2e9
0cb4e0b
5476b65
71ac9d0
365214d
6f6c5d3
e96c0ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ language: node_js | |
node_js: | ||
- "6" | ||
|
||
sudo: false | ||
sudo: required | ||
dist: trusty | ||
|
||
addons: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
/* eslint-env node */ | ||
|
||
const { existsSync } = require('fs'); | ||
const path = require('path'); | ||
|
||
module.exports = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
// @ts-check | ||
/* eslint-env node */ | ||
|
||
const child_process = require('child_process'); | ||
const fs = require('fs'); | ||
|
||
const rimraf = require('rimraf'); | ||
|
||
module.exports = (project, outDir) => { | ||
const Serve = project.require('ember-cli/lib/commands/serve'); | ||
const Builder = project.require('ember-cli/lib/models/builder'); | ||
const Watcher = project.require('ember-cli/lib/models/watcher'); | ||
|
||
/** | ||
* Exclude .ts files from being watched | ||
*/ | ||
function filterTS(name) { | ||
if (name.startsWith('.')) { | ||
// these files are filtered by default | ||
return false; | ||
} | ||
|
||
// typescript sources are watched by `tsc --watch` instead | ||
return !name.endsWith('.ts'); | ||
} | ||
|
||
class WatcherNonTS extends Watcher { | ||
buildOptions() { | ||
let options = super.buildOptions(); | ||
options.filter = filterTS; | ||
return options; | ||
} | ||
} | ||
|
||
return Serve.extend({ | ||
name: 'serve-ts', | ||
aliases: ['st'], | ||
works: 'insideProject', | ||
description: | ||
'Serve the app/addon with the TypeScript compiler in incremental mode. (Much faster!)', | ||
|
||
run(options) { | ||
const config = this.project.config(options.environment); | ||
|
||
this.project._isRunningServeTS = true; | ||
|
||
return new Promise(resolve => { | ||
let started = false; | ||
|
||
this.ui.startProgress('Starting TypeScript compilation...'); | ||
|
||
// TODO: typescript might be installed globally? | ||
// argument sequence here is meaningful; don't apply prettier. | ||
// prettier-ignore | ||
this.tsc = child_process.fork( | ||
'node_modules/typescript/bin/tsc', | ||
[ | ||
'--watch', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dwickern @dfreeman one thing I've been thinking is: we should be able to get and use the project's tsconfig, and then use that here (using Does that make sense to both of you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternative approach: do the usual Also, I don't think we need to worry about using a global install of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it already picks up the tsconfig in the project root based on the cwd? We could pass the path explicitly though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I just went back and read the docs, and you're both correct about those pieces. So here, we should just be able to override only what we need to – and presumably, other than outdir, that should largely match what we have in the overrides in the Note that that's assuming valid JSON. 🤔 |
||
'--outDir', outDir, | ||
'--allowJs', 'false', | ||
'--noEmit', 'false', | ||
'--sourceMap', 'false', // TODO: enable if sourcemaps=true | ||
], | ||
{ | ||
silent: true, | ||
execArgv: [], | ||
} | ||
); | ||
|
||
this.tsc.stderr.on('data', data => { | ||
this.ui.writeError(data); | ||
}); | ||
|
||
this.tsc.stdout.on('data', data => { | ||
this.ui.write(data); | ||
|
||
// Wait for the initial compilation to complete before continuing to | ||
// minimize thrashing during startup. | ||
if (data.indexOf('Compilation complete') !== -1 && !started) { | ||
started = true; | ||
this.ui.stopProgress(); | ||
resolve(); | ||
} | ||
}); | ||
}).then(() => { | ||
const builder = new Builder({ | ||
ui: this.ui, | ||
outputPath: options.outputPath, | ||
project: this.project, | ||
environment: options.environment, | ||
}); | ||
|
||
// This will be populated later by the superclass, but is needed by the Watcher now | ||
options.rootURL = config.rootURL || '/'; | ||
|
||
// We're ignoring this because TS doesn't have types for `Watcher`; this is | ||
// fine, though. | ||
// @ts-ignore | ||
const watcher = new WatcherNonTS({ | ||
ui: this.ui, | ||
builder, | ||
analytics: this.analytics, | ||
options, | ||
serving: true, | ||
}); | ||
|
||
options._builder = builder; | ||
options._watcher = watcher; | ||
|
||
return Serve.prototype.run.call(this, options); | ||
}); | ||
}, | ||
|
||
onInterrupt() { | ||
return Serve.prototype.onInterrupt.apply(this, arguments).then(() => { | ||
if (this.tsc) { | ||
this.tsc.kill(); | ||
} | ||
|
||
if (fs.existsSync(outDir)) { | ||
rimraf.sync(outDir); | ||
} | ||
}); | ||
}, | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue described here and here on Travis' end.