Implement fast incremental compiler #85

dwickern · 2017-11-24T00:38:10Z

The idea here is to use tsc --watch instead of broccoli-typescript-compiler, since tsc has much faster incremental compile logic. There is no public API for the incremental compiler yet, but once microsoft/TypeScript#9282 is implemented, broccoli-typescript-compiler should be equally fast and this PR will be obsolete.

There are lots of issues with my hacked together implementation

the .ts are still being watched, so each change triggers two rebuilds. Fixing this should make it faster.
ember-cli will not watch a directory in tmp so I put the compiled js into an .e-c-ts directory
the incremental compiler should only be used in development, but I couldn't figure out how to check the environment
I couldn't figure out the broccoli hook to use to clean up temp files and child processes
I'm swallowing all the stdout from tsc since the error messages are nasty, e.g.

tmp/ember_cli_typescript_fast_watcher-input_base_path- U6assc9z.tmp/web/tests/acceptance/search-test.ts(55,7): error TS2304: Cannot find name 'server'.

chriskrycho

😍 – I'll see if I can take a stab at some of the other pieces, perhaps this afternoon or tomorrow.

chriskrycho · 2017-11-24T16:22:07Z

lib/typescript-fast-incremental-preprocessor.js

+
+function makeTempDir() {
+  // TODO: the directory won't get monitored if it's in tmp/
+  const dir = '.e-c-ts';


This seems like a reasonable compromise to me; we'll just want to do two things if we run with it:

Document it.

Figure out how to automatically update .gitignore on installing the app, so that this directory is always excluded.

chriskrycho · 2017-11-24T16:23:50Z

lib/typescript-fast-incremental-preprocessor.js

+      annotation: 'JS files',
+    });
+
+    // TODO: how to disable watching .ts files?


Once we figure out how to correctly identify the environment, it should either be available in the same way here or we can use it to set a flag on the this instance that we pass in so that we watch or not as appropriate for the environment.

Alternatively, maybe in the fast-compiler we intentionally just ignore these files entirely and only trigger on the files in the .e-c-ts directory changing?

dwickern · 2017-11-24T17:46:27Z

I think the main problem are the two rebuilds

I modify foo.ts
ember-cli file watcher sees the change in foo.ts and triggers a rebuild
rebuild copies foo.ts to tmp/.../foo.ts using broccoli-funnel
tsc --watch file watcher sees the change in tmp/.../foo.ts
tsc --watch recompiles and outputs .e-c-ts/foo.js
ember-cli file watcher sees the change in foo.js and triggers a rebuild

It's a bit faster than broccoli-typescript-compiler but still not as fast as running tsc --watch to compile in-place. I think I need to take a different approach...

chriskrycho · 2017-11-24T17:50:49Z

Ah, I misunderstood part of the flow in practice (a bit aspirationally I suppose). My assumption was (and I think if we could get there it would make sense if) it did:

Modify foo.ts.
tsc --watch recompiles and outputs .e-c-ts/foo.js.
ember-cli file watcher sees change in foo.js and triggers rebuild, which funnels to correct tmp directory.

The main challenge I'd foresee with that approach would be getting the bits from .e-c-ts into the correct tmp directory with the funnel, but I think you could map it correctly? (My broccoli knowledge is… not large.)

dwickern · 2017-11-24T18:35:10Z

The advantage of using the preprocessor hooks is they have already sorted out and merged all the trees.

In your step 2, ember-cli also sees the change to foo.ts and triggers the second recompile. I think the only way to avoid this will be using a custom command, e.g. ember serve-ts

chriskrycho · 2017-11-24T20:15:17Z

Mmmm. 🤔 @rwjblue, any suggestions here?

dwickern · 2017-11-25T18:25:35Z

I changed this to use a separate command instead so that I can tweak the watcher

ember serve-ts

It works! and it's really fast, as if you're not using typescript at all

chriskrycho · 2017-11-25T18:51:28Z

Sweet! I’ll test it out and we can ship it!

chriskrycho · 2017-11-25T19:36:20Z

These test failures are incredibly annoying. Nothing changed on our end for them. 😑

chriskrycho · 2017-11-26T00:02:51Z

Two quick notes:

I pulled this down and tried to use it in our main app repository, and the TS watcher needs to exclude the blueprints directory. Otherwise, you end up with some nasty errors when using the substitutions that templates allow. We have a template that looks like this:

import Component from '@ember/component';

import { bem } from 'mobile-web/lib/utilities/bem';

export const block = '<%= dasherizedModuleName %>';

export default class <%= classifiedModuleName %> extends Component.extend({
  // `attributeBindings` and `classNameBindings` must be here until we have TS
  // support for the new decorator spec, as they need to be merged with other
  // items of the same name in the prototype chain.
  attributeBindings: [`elementId:data-test-${block}`],

  // You should remove `classNameBindings` and `tagClass` if you're going to
  // apply the BEM root to an element in the template explicitly.
  classNameBindings: ['tagClass'],
}) {

  tagClass = bem({ block });
}

It's named blueprints/bem-component/files/app/components/__name__.ts (and there's a matching one for the integration test). Compilation falls over with this error:

SyntaxError: mobile-web/blueprints/bem-component/files/app/components/__name__.js: Unexpected token (6:1)
  4 | export default class {
  5 | }
> 6 |  %  > ;
    |  ^
  7 | Component.extend({
  8 |     // `attributeBindings` and `classNameBindings` must be here until we have TS
  9 |     // support for the new decorator spec, as they need to be merged with other

It makes sense: it is a syntax error, but... we just need to exclude the blueprints directory.

We need to make sure we always delete the .e-c-ts directory after a run. Otherwise, problems like the one above will stick around after renames, etc.

chriskrycho · 2017-11-26T00:19:32Z

Next comment: holy smokes this is fast.

chriskrycho · 2017-11-26T02:09:14Z

Played with it a good bit; other than needing that fix for the blueprints directory and getting rid of the .e-c-ts directory, it's amazing.

So, to ready this for shipping it:

add .e-c-ts to .gitignore on install
clean up by deleting .e-c-ts when shutting down
ignore the blueprints directory
document the serve-ts command

Edit: heh, and maybe also:

don't swallow type errors

dwickern · 2017-11-26T02:40:00Z

Maybe blueprints/ should be excluded in tsconfig? Or better yet, we could use explicit includes. I had similar issues before I disabled allowJs because it was compiling stuff in server/

dwickern · 2017-11-26T02:42:28Z

We have actually 2 separate builds: running tsc directly (with serve-ts or from the IDE) and the broccoli pipeline build which does a bunch of transformations first. And both builds need to compile...

dwickern · 2017-11-26T02:44:20Z

I also figured out why I couldn't put .e-c-ts into tmp: the directory is completely ignored in the watchman config.

chriskrycho · 2017-12-17T22:53:03Z

@dwickern do you know what we need to do so we're not swallowing type errors when doing these incremental builds? I'd like to knock this out and ship it, and I think I know how to tackle the rest of it, but am unsure why those are getting swallowed at present. 🤔 Hopefully we can ship this as 1.1 this week, and ship the addon support soon as well.

Implement `Command#onInterrupt` to remove directory during shutdown. Also add prettier flag not to tweak the layout of tsc process fork.

Implement `Command#onInterrupt` to remove directory during shutdown. Also add prettier-ignore for the tsc process creation, since arguments are *not* like ordinary arrays.

dwickern · 2017-12-18T00:45:34Z

I get an error on startup now

Error: process is not captured
    at Object.addHandler (/Users/dwickern/code/ember-cli-typescript/node_modules/ember-cli/lib/utilities/will-interrupt-process.js:82:13)
    at new Builder (/Users/dwickern/code/ember-cli-typescript/node_modules/ember-cli/lib/models/builder.js:37:30)
    at Class.run (/Users/dwickern/code/ember-cli-typescript/lib/serve-ts.js:41:21)
    at resolve (/Users/dwickern/code/ember-cli-typescript/node_modules/ember-cli/lib/models/command.js:317:20)
    at initializePromise (/Users/dwickern/code/ember-cli-typescript/node_modules/ember-cli/node_modules/rsvp/dist/rsvp.js:532:5)

chriskrycho · 2017-12-18T01:13:01Z

Huh. I’ll straighten it out tomorrow. Not sure which of my changes would have caused that. 🤔

This reverts commit 2feb95a.

Also fix things when linking

Solve 'The Mystery of the Missing app.js 🕵️‍♂️'

chriskrycho · 2018-01-24T20:29:20Z

.travis.yml

@@ -3,7 +3,7 @@ language: node_js
 node_js:
  - "6"

-sudo: false
+sudo: required


Issue described here and here on Travis' end.

Assorted improvements

chriskrycho · 2018-01-24T20:39:40Z

lib/serve-ts.js

+        this.tsc = child_process.fork(
+          'node_modules/typescript/bin/tsc',
+          [
+            '--watch',


@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 -p <path to project>) except with overrides. I believe TS properly does the override sequence if you just hand it a project file and then a set of overrides.

Does that make sense to both of you?

Alternative approach: do the usual JSON.parse(tsConfig) dance and set only the properties we want to set here, e.g. sourceMap: <whatever is in the config That might be better.

Also, I don't think we need to worry about using a global install of tsc; we should only be using the local version, since that's what will be used for the final build, as well.

I like the idea of using -p <path-to-project> and overriding with the handful of flags we need — it feels simpler (and I also think tsc will actually let you get away with nonconformant JSON, so we'd need something fancier than JSON.parse for that approach)

I think it already picks up the tsconfig in the project root based on the cwd? We could pass the path explicitly though.

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 TypescriptPreprocessor class (starting here).

Note that that's assuming valid JSON. 🤔

chriskrycho · 2018-01-25T00:06:34Z

Two things:

Still seeing the problem of the tree getting pulled twice somehow. I end up seeing output like this for ember serve:

WARNING: more-ts/Users/chris/dev/oss/more-ts/types/ember-cli-mirage/index.d.ts(220,16): error TS2451: Cannot redeclare block-scoped variable 'server'.
types/ember-cli-mirage/index.d.ts(220,16): error TS2451: Cannot redeclare block-scoped variable 'server'.

Best bet there is just that the treeForApp is getting something funky.

When I run ember serve-ts locally, I see:

    /Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:58192
                throw e;
                ^

TypeError: Cannot read property 'escapedText' of undefined
    at writeClassDeclaration (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:53824:84)
    at writeModuleElement (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:53428:28)
    at emitModuleElement (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:53389:17)
    at emitNode (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:54363:28)
    at emitLines (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:53136:17)
    at emitSourceFile (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:53335:13)
    at /Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:52928:17
    at Object.forEach (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:281:30)
    at emitDeclarations (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:52910:12)
    at Object.writeDeclarationFile (/Users/chris/dev/oss/more-ts/node_modules/typescript/lib/tsc.js:54418:37)

Just logging those here in case either of you gets to them ahead of me. I'll be at it again tomorrow.

…-cli commands work

Incremental compiler with broccoli integration

Implement fast incremental compiler

implement fast incremental compiler

62a81de

chriskrycho reviewed Nov 24, 2017

View reviewed changes

use serve-ts command instead of preprocessor

63d5e1e

Update lockfile and ember-try config.

e43d55b

dwickern added 2 commits November 25, 2017 12:32

fix first-time directory creation

b8248d9

add flag when running with serve-ts

0362674

chriskrycho added 2 commits December 17, 2017 15:32

Merge in 1.0.6 (Ember upgrade) changes.

1db94d5

Exclude blueprints from build.

0955fc3

chriskrycho added 5 commits December 17, 2017 17:22

Enable node type-checking with @ts-check.

18774d7

Fix lint and type errors in typescript-preprocessor.

39a53d3

Cleanup of .e-c-ts dir when done with ts-serve.

3da13f9

Implement `Command#onInterrupt` to remove directory during shutdown. Also add prettier flag not to tweak the layout of tsc process fork.

Implement cleanup of .e-c-ts dir.

64484df

Implement `Command#onInterrupt` to remove directory during shutdown. Also add prettier-ignore for the tsc process creation, since arguments are *not* like ordinary arrays.

Clean up tsc process on ^C.

2a2952b

chriskrycho force-pushed the fast-incremental-compiler branch from 3f49802 to 2a2952b Compare December 18, 2017 00:38

chriskrycho and others added 11 commits January 15, 2018 17:26

Revert "Try using cross-env for node paths."

d5147d2

This reverts commit 2feb95a.

Handle types dir by convention.

59ab4fd

Include *all* directories included via include as well.

8c42b49

Allow use of environment variable for npm link scenario.

624b70c

Solve 'The Mystery of the Missing app.js 🕵️‍♂️'

77f13e5

Also fix things when linking

Merge pull request #89 from dfreeman/fast-incremental-compiler

3f722d9

Solve 'The Mystery of the Missing app.js 🕵️‍♂️'

Fix cleanup

da18bba

Move incremental output to OS temp

54edcdc

Minimize rebuild thrashing

8fad31c

Fix app URL in serve output

0e4b8b5

Meltdown aftermath: sudo required in Travis.

fe5035e

chriskrycho reviewed Jan 24, 2018

View reviewed changes

Merge pull request #90 from dfreeman/fast-incremental-compiler

a666892

Assorted improvements

chriskrycho reviewed Jan 24, 2018

View reviewed changes

Better approach for Meltdown mitigation Chrome handling.

0b4b4fe

chriskrycho force-pushed the fast-incremental-compiler branch from 1b45638 to 0b4b4fe Compare January 24, 2018 20:44

Do not override project-level sourcemap setting.

59e2c77

dfreeman and others added 9 commits February 4, 2018 18:59

Avoid double-including files

ea8a494

Spike out a Broccoli-integrated incremental compiler so regular ember…

b70d390

…-cli commands work

Fix addons' app trees and cleanup dead code

bdde2e9

Merge pull request #92 from dfreeman/incremental-with-broccoli

0cb4e0b

Incremental compiler with broccoli integration

Fix the dummy app

5476b65

Merge branch 'master' into fast-incremental-compiler

71ac9d0

Fix rebuilds triggered by non-TS files.

365214d

At least give tsc a *chance* to catch up

6f6c5d3

writeErrorLine -> writeError. Also: Prettier.

e96c0ca

chriskrycho merged commit 3161a06 into master Feb 6, 2018

chriskrycho deleted the fast-incremental-compiler branch February 6, 2018 15:33

dfreeman pushed a commit that referenced this pull request Aug 21, 2018

Merge pull request #85 from typed-ember/fast-incremental-compiler

399e2b2

Implement fast incremental compiler

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fast incremental compiler #85

Implement fast incremental compiler #85

dwickern commented Nov 24, 2017

chriskrycho left a comment

chriskrycho Nov 24, 2017

chriskrycho Nov 24, 2017

dwickern commented Nov 24, 2017

chriskrycho commented Nov 24, 2017

dwickern commented Nov 24, 2017

chriskrycho commented Nov 24, 2017

dwickern commented Nov 25, 2017

chriskrycho commented Nov 25, 2017 via email

chriskrycho commented Nov 25, 2017

chriskrycho commented Nov 26, 2017

chriskrycho commented Nov 26, 2017

chriskrycho commented Nov 26, 2017 •

edited

Loading

dwickern commented Nov 26, 2017

dwickern commented Nov 26, 2017 •

edited

Loading

dwickern commented Nov 26, 2017

chriskrycho commented Dec 17, 2017

dwickern commented Dec 18, 2017

chriskrycho commented Dec 18, 2017 via email

chriskrycho Jan 24, 2018

chriskrycho Jan 24, 2018

chriskrycho Jan 24, 2018

dfreeman Jan 24, 2018

dwickern Jan 24, 2018

chriskrycho Jan 24, 2018

chriskrycho commented Jan 25, 2018

Implement fast incremental compiler #85

Implement fast incremental compiler #85

Conversation

dwickern commented Nov 24, 2017

chriskrycho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwickern commented Nov 24, 2017

chriskrycho commented Nov 24, 2017

dwickern commented Nov 24, 2017

chriskrycho commented Nov 24, 2017

dwickern commented Nov 25, 2017

chriskrycho commented Nov 25, 2017 via email

chriskrycho commented Nov 25, 2017

chriskrycho commented Nov 26, 2017

chriskrycho commented Nov 26, 2017

chriskrycho commented Nov 26, 2017 • edited Loading

dwickern commented Nov 26, 2017

dwickern commented Nov 26, 2017 • edited Loading

dwickern commented Nov 26, 2017

chriskrycho commented Dec 17, 2017

dwickern commented Dec 18, 2017

chriskrycho commented Dec 18, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho commented Jan 25, 2018

chriskrycho commented Nov 26, 2017 •

edited

Loading

dwickern commented Nov 26, 2017 •

edited

Loading