frankwallis / plugin-typescript

TypeScript loader for SystemJS
MIT License
248 stars 47 forks source link

caching feature request #133

Closed bmayen closed 8 years ago

bmayen commented 8 years ago

Initial build time for a simple Angular2 app sfx bundle takes a few seconds to complete in my environment. Using SystemJS built-in typescript support, subsequent watch builds with caching enabled take milliseconds. However, using plugin-typescript all subsequent builds take just as long as the first.

I brought this up with Guy regarding another plugin and he had some useful feedback here: https://github.com/systemjs/builder/issues/593

Would love to have caching enabled in this plugin. Any idea what the level of effort would be? I'd be happy to help implement if I can.

frankwallis commented 8 years ago

TBH I would expect this to work so I will need to investigate to see what is going on, if you are able to update the angular2 example project with your configuration demonstrating the issue then that would help.

plugin-typescript loads all its files through SystemJS (ie. it does not use System.fetch or fs.readFile) so I would expect SystemJS to handle the caching of these files. Anyway, I will have a play around with it and try and work out why it is taking so long.

bmayen commented 8 years ago

Will do. Unfortunately, I might not be able to get to this until after the weekend because I have company in town. If not before then, will have it to you ASAP next week.

bmayen commented 8 years ago

@frankwallis, sorry for the delay on this. I have added a very basic gulp build to illustrate the issue using the Angular2 example at https://github.com/bmayen/plugin-typescript/tree/feature/caching_example.

Run gulp watch to start a watch then observe how all subsequent task runs take just as long as the first. If I change this example to use JSPM's default Typescript transpiler, subsequent runs instead take milliseconds.

frankwallis commented 8 years ago

Thanks for the repro, I can see the issue, so I just need to work out the best way forward.

bmayen commented 8 years ago

@frankwallis, any thoughts on this? Perhaps something I could help out with? Or maybe a workaround in the meantime? As our project grows, this is moving into the critical path for us; really feeling the impact of the full compilation times for the smallest of changes.

frankwallis commented 8 years ago

My view on this is that SystemJS does not cache the output from plugins, only from the internal transpiler. So I think the same issue exists with plugin-babel. Perhaps there needs to be a way for a plugin to notify SystemJS that the output is cacheable, by setting a property on the metadata?

@guybedford interested to know your thoughts..

guybedford commented 8 years ago

@frankwallis the output of plugins is cached in SystemJS builder. This works by setting load.metadata.timestamp in the fetch hook for a module and if it matches the timestamp of the file at load.path then the load does not get recompiled in the builder, it keeps the compiled source in the cache.

I'm aiming to extend this API to handle more complex cases of caching and subtrees, so let me know where the above model might be breaking down here?

Also @frankwallis no pressure but it would be nice to discuss the mechanism here if you have some time at some point to chat on Gitter as I'd really appreciate bouncing off some of the ideas around the builder API evolution and how they apply to this use case.

frankwallis commented 8 years ago

@guybedford i'm happy to discuss, although I actually think the current API should be sufficient for my purposes. I have investigated this a bit more and there are a couple of issues which are causing this not to work:

1) load.metadata.timestamp is not being set in System.fetch Looking at https://github.com/systemjs/builder/blob/master/lib/builder.js#L179 I don't think that the callback function is ever being called as builder.fetch/loaderFetch is returning a promise. I had a look at trying to fix it myself but it caused some of the tests to fail so I think it might be better if you could take a look.

2) plugin-typescript not present in loads map In isLoadFresh I am seeing this line being hit: https://github.com/systemjs/builder/blob/master/lib/trace.js#L97 because github:frankwallis/plugin-typescript.../plugin.js is not a key in the loads map. I am not really sure why this is though. I will look a bit further.

If I hack in fixes to these 2 issues then I can see that the caching does start working and the build time drops significantly.

guybedford commented 8 years ago

@frankwallis for the use of System.fetch you can try using fetch: function(load, fetch) and call the fetch function passed as the second argument there which I believe should provide caching. Otherwise you can always set load.metadata.timestamp manually.

For the loads map this is because the build is based on two loaders. A loader for the trace and a loader for plugins in the build. So the plugin itself only exists as an instance as long as the builder instance, but should be cached at least within that instance until reset is called.

For the more general API, I'll aim to keep you posted on directions here.

frankwallis commented 8 years ago

@guybedford the timestamp issue (1 above) is now resolved, but I am not sure how to proceed on (2).

https://github.com/systemjs/builder/blob/master/lib/trace.js#L95:

  if (load.plugin) {
    var plugin = loads[load.plugin];
    if (!isLoadFresh(plugin, loader, loads))
      return false;
  }

It seems to me that here plugin will always be undefined, so isFileFresh will return false for any file loaded by a plugin. Is there something I am missing?

bmayen commented 8 years ago

Not sure if this is related or not, but after updating to latest JSPM beta.23 and latest plugin-typescript, I am running into an issue where subsequent watch runs cannot find local modules referenced with a relative path. It compiles them fine on the initial run, but after making changes and triggering a build from watch, it complains "cannot find module './path/to/local/src' (TS2307)". It does not complain about imports for packages like @angular/core, just the imports referenced with a relative path.

bmayen commented 8 years ago

@frankwallis can you confirm if this is a related issue with the plugin, or if it's further up the chain? Our builds are broken so we've reverted back to previous versions of JSPM and the plugin for now, but I'd like to report this issue correctly.

Thanks again for all the work on this!

frankwallis commented 8 years ago

@bmayen I wasn't able to recreate this (I was trying in the caching_example project) so it's hard to know where the actual problem is. If you can recreate it somewhere then I will happily take a look.

bmayen commented 8 years ago

Thanks. Will have a look tonight and see if I can recreate it there.

bmayen commented 8 years ago

Sorry for the delay on this.

Demonstrating the issue here https://github.com/bmayen/plugin-typescript/tree/feature/caching_example. If you run gulp watch and make a couple of edits to "calculator-component.ts", you'll see the error. "TypeScript [Error] file:///plugin-typescript/examples/angular2/src/calculator-component.ts:2:48 TypeScript [Error] Cannot find module './calculator-store'. (TS2307)"

In this example, it doesn't happen after the first edit because that causes the initial build. But after the second edit, when we use the caching, something happens that causes "calculator-store" to get lost.

bmayen commented 8 years ago

@frankwallis, I can confirm that if I disable typechecking the bug reported above goes away and subsequent watch runs take far less time. Initial build takes about 10s in our project whereas the cached execution takes ~2s. Surprisingly though, this is still much longer than the SystemJS built-in compiler, which takes milliseconds once cached.

With typechecking enabled, and ignoring the "cannot find module" bug, subsequent watch runs take ~6s. So for now our workflow is to run dev environments without typechecking and manually run one typechecked build before committing. If we forget, CI always runs with typechecking and catches it there. Of course, the ideal would be to get the cached typechecked builds down to a more reasonable execution time :) I'm sure we'll get there!

If there's anything else I can do to help track this down, please let me know.

frankwallis commented 8 years ago

I have investigated this some more and the issue is that plugin-typescript is unloaded and reloaded each time the build is run, and so all the state held by the type-checker is lost between runs and the files have to be rechecked each time. I don't see any easy way around this at all unfortunately. I need a way to make the compiler-host persist between builds.

bmayen commented 8 years ago

Is/could this state held in a single object? Sounds like this is something that could be added to SystemJS fairly easily. Perhaps as an additional param to setCache? If this sounds reasonable, I can start a separate discussion over there.

guybedford commented 8 years ago

@frankwallis if the plugin is reloaded each time, would you be able to serialize and deserialize this internal state if access to a global cache store was provided? Otherwise if keeping the same plugin instance is the way forward, we need to ensure we have suitable invalidation hooks. I'd be happy with either direction here, just let me know what you think might be best?

frankwallis commented 8 years ago

The state is not serializable AFAIK, it is the complete TypeScript compiler instance which I don't have any control over. If there was a way I could 'break out' of SystemJS and store a variable in NodeJS global that might be a way to do it?

guybedford commented 8 years ago

@frankwallis apologies for the tangent here, your initial analysis in https://github.com/frankwallis/plugin-typescript/issues/133#issuecomment-235412928 was completely spot on. I've added a possible fix for this in https://github.com/systemjs/builder/commit/8ff47b78d5043f59efe20c94470036dfa90f58df, let me know if that helps here?

frankwallis commented 8 years ago

@guybedford I've tried it and I think it actually needs to look like this:

if (load.plugin) {
  if (!loader.pluginLoader.has(loader.pluginLoader.decanonicalize(load.plugin)))
    return false;
}

And that does massively reduce the rebuild time :) It doesn't solve the later issue that a fresh instance of the plugin is used for each rebuild however :(

frankwallis commented 8 years ago

@guybedford Actually I think I may have figured out what is going on here - the first time around everything works ok. The second time around a new instance of the plugin is created and used for the translate calls, however the original instance is used for the bundle call.

I know that the bundle hook is only called when the plugin is configured as a loader. This makes me think that the issue is to do with the plugin being configured both as the SystemJS default transpiler as well as a loader.

I also see the first time around that the bundle hook is called twice, although that may not be an issue or related to this.

Hope this makes sense.

guybedford commented 8 years ago

@frankwallis thanks I've added the fix.

That sounds exactly right actually. I've added some experimental commits in https://github.com/systemjs/systemjs/commit/8fa13b56697251f70ed2911bdb78406dc5b46a97 and https://github.com/systemjs/builder/commit/5706c101aa994d502284df4e322c066cf5ae3b19.

I haven't got a massive amount of time to test it more deeply, but that seems to then trigger the plugin behaviours in the builder for the load.

If you're able to test it out further please do let me know.

frankwallis commented 8 years ago

I'm not clear what the effect was supposed to be, but it did't seem to have much effect, a new instance of the plugin was still created and different instances were used for the bundle and translate hooks on the second build.

I have added a workaround in 5.2.5 which persists the compiler between instantiations of the plugin so hopefully that will suffice for now once the isFresh changes are released.

frankwallis commented 8 years ago

It looks like it is calling the bundle hook now when only configured as the default transpiler though?

guybedford commented 8 years ago

@frankwallis it's definitely calling the bundle hook in both cases in my tests, are you sure you applied the patch correctly? (note the SystemJS patch needs make build to be run as well)

As for the plugin instances, I'm not quite sure I follow what you mean that it generates different instances for bundle and translate? SystemJS builder just calls those hooks.

guybedford commented 8 years ago

@frankwallis perhaps the instances are different because a builder.reset() is being run here? In which case, yes a new plugin instance would be used. But if builder.invalidate('*') is being run, it should still be the same plugin instance.

guybedford commented 8 years ago

Actually sorry yes of course builder.invalidate('*') will invalidate the plugin itself. To avoid this use a wildcard form that matches the sources but not the plugin, eg - builder.invalidate('src/*'). I'm wondering how we can distinguish the plugin for invalidation here...

guybedford commented 8 years ago

A global cache store may just be the best approach barring partial invalidation, as the plugin is part of the invalidation set in the architecture. The bundle hook still using the old instance is a bug though... would be interesting to track that down.

frankwallis commented 8 years ago

I believe this is all now working in jspm@0.17.0-beta.31 and plugin-typescript@5.2.9.

The issue with the bundle hook also seems to be resolved, its also possible it was a phantom issue caused by me not rebuilding systemjs.