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

[WIP] Typescript Compilation + Exploring editor extensions #614

Merged
merged 40 commits into from Apr 1, 2016

Conversation

Projects
None yet
3 participants
@shaddockh
Copy link
Collaborator

shaddockh commented Jan 16, 2016

This PR is more of an experimental dive into trying to provide the ability to transpile TypeScript on save of a .ts file in the editor as well as playing with an editor extension type of architecture.
TypeScriptLanguageService.ts
This shows two ways of handling typescript. Either a simple single file transpile, which is very fast, but will only catch blatant syntax issues, or a full compile which loads all the TS files in the project and runs through full type checking etc. This second option runs pretty slowly, and maybe either that can only be done on request, or during the build process.

Typescript.js
this is a customized version of typescript.js that fixes a regex issue that duktape doesn't like. The BuildCommon.js copies this file into the modules folder on build.

Extension Services
Basically what is happening is that there is ServiceLocator that extensions can register with. When editor events occur, they notify the ServiceLocator that the event happened and the ServiceLocator delegates to anything registered for that event that can handle it. Right now only the SaveResource method is hooked in, but this could be hooked into menu rendering, inspector creation, even filetype resource editors if it got robust enough.

Some thoughts behind this were that you could have a Resources/EditorExtensions folder that could dynamically be queried by the editor to register new extensions. In this example, the TypeScript resource extension is manually loaded in. Also, it might be better to somehow hook into the normal event subscription model to notify these extensions of changes.

Anyway, this is definitely a WIP and needs some discussion and iteration to make it better.

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 17, 2016

Holy cow! This is going to be great :)

I'll look into it soon, in the meantime, if you can spare a comment in this Duktape thread about the regex issue you ran into on "this is a customized version of typescript.js that fixes a regex issue that duktape doesn't like"?

svaarala/duktape#523

They may already have a fix...

@fatcerberus

This comment has been minimized.

Copy link

fatcerberus commented Jan 17, 2016

@JoshEngebretson There is indeed a fix in the pipeline for the regex issue:

svaarala/duktape#513

With that, the TS compiler should be able to be used unmodified. It's related to literal curly braces in a regex, which is technically in violation of Ecmascript spec but is taken as given by JS developers.

@fatcerberus

This comment has been minimized.

Copy link

fatcerberus commented Jan 17, 2016

It was merged. :)

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 17, 2016

@fatcerberus Thanks for the heads up

@shaddockh We have the latest and greatest Duktape with the Regex updates on master as of #617, this will hopefully support stock TS compiler and if you can build in release, would be curious if you notice any performance improvements (FYI: glew.c takes forever to compile in release on Apple clang for some reason)

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Jan 17, 2016

I can confirm that the latest Duktape fixes work and Typescript is loading into the Duktape vm w/o modifiation.

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 17, 2016

I just breezed over the scripts and they look really good. I have to level my TypeScript skills up a bit it would appear 😊

We really do need to build per project extensions into the editor, possibly even have global extensions. I am balancing a few issues now, though I would love to jam on this some over the next week.

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Jan 18, 2016

This latest commit turns on full project compilation on save. It's a little slow, so may need some instrumentation but what it does is:

  • First time it's executed, it compiles all .ts files
  • any subsequent save will have it only compile the file that is saved

I needed to add a couple files to: App/Resources/ToolData/TypeScriptSupport to bring in the core typescript types (lib.core.d.ts) copied from the typescript distribution and I created a duktape.d.ts that will contain duktape builtins. I believe the build script does this correctly, but I'm not sure if there is a specific order files get copied, so may need some tweaking.

In the project, the scanner looks for all .ts files under Resources and also any d.ts files under typings. I'm thinking that maybe typings should move under Resources, but I'm not sure yet.

Some todo items I can think of:

  • hook into project open so the state of the typescript service can get reset
  • detect files deleted or moved in the project so that the internal state can get updated
@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 18, 2016

More great stuff :)

I am thinking performance is going to be pretty important for autocomplete? I guess need to look into how ts handles it. I believe having separate duk_context's is thread safe, so having a separate Duktape vm on a background thread could make this doable (and keep the main editor VM clean of loading all this into it). How long does the full compile take on the dungeon scripts?

If we can keep this in process that would be a win, though we could write this in TS/JS and feed to node... dumping back via either stdout, file marshal (json warning/errors/success) or IPC. In process would be good for autocomplete I think, unless we can launch a node service to do it...

The important thing is that we're on the right path 👍

BTW, I wrote the JavaScript mod support for Contraption Maker: http://store.steampowered.com/app/241240/

... and used Duktape coroutines to break up the processing on syntax highlighting code which was in JS, the power of "yield" :)

@fatcerberus

This comment has been minimized.

Copy link

fatcerberus commented Jan 18, 2016

Multiple heaps should be threadsafe, as long as two threads don't try to access the same one simultaneously. Be careful here though--duk_contexts themselves are not necessarily threadsafe, as I believe it's possible to have more than one context referring to the same heap.

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 18, 2016

@fatcerberus Thanks, that's right I meant heap, would be a good test case on this theory in any event :)

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Jan 18, 2016

For timings: On a 2009MBP, a full compile of the dungeon example takes 10.7 seconds. Once it has that state built up, compiling just main.ts takes 3.5 seconds. These times definitely indicate that doing this on a background thread will be necessary.

If there is a way to call across threads or register events that can be passed between them, that might be a good option. At the very least there would need to be something that could tell the compiler service to:

  • reset
  • full compile
  • individual file compile
  • retrieve any errors

All of the file paths and any information could be passed directly to the thread so that it doesn't need to call back to the editor thread for any additional information...just to notify it of any errors.

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Jan 20, 2016

Just a little time spent on the extension registries. There is now a ResourceService interface which listens to changes on the resources and a ProjectService interface which at this point just listens for Project load and unload. I also put ina demonstration of how the TypescriptLanguageService can implement both interfaces and register with both service registries to listen to both types of events.

There could have just had a single registry with all types of services, and is pretty trivial to do, but I kind of liked having specific registries for resource vs project events so that they are only called when necessary.

What I'm wondering also, is if there is a way to build up a service type that knows that it needs to run on a different thread and just pass the relevant information across. Ideally, each of these services would run in an isolated thread so they don't have a chance to crash the editor, but I'm not really sure what that would look like.

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 23, 2016

Great, and see you're delving into the C++ too 👍

Threading a single duktape heap won't really work, though we should be able to thread across multiple heaps, could also use coroutines to cooperatively multitask, you really can do a lot with them to break up long running jobs.

I have a suspicion that we'll need to use node via the subprocess system for performance reasons on compile/autocompletion and that this could be a simpler approach than threading.

I am working on getting the UIWebView (and new code editor) up on master, which will be interesting to get rolling with this, in some ways "closing the loop" :)

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Jan 23, 2016

I've been wanting to get into co-routines, now I have a good excuse. :)

In these last commits, one thing that I've tried to do is make all the communication between the editor and the services be through the send/subscribe event model. Hopefully this means that the services can be either run on a background thread or even sandboxed to some extent.

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 31, 2016

The WebView is now on master with some known issues (like enter not working to select the autocompletion in editor), arrow keys not working in google search suggestions, links that open a new tab/window open a top level window, etc. It looks like this branch now has some merge conflicts, hopefully not terrible to sort, let me know if you run into anything rebasing to master

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Jan 31, 2016

Cool, I'll rebase and pull everything in tomorrow. Excellent work, btw. :)

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Jan 31, 2016

Thanks, lots of heavy lifting is done, should be relatively easy at this point to fill in the API we need... and address bugs. The fun part is coming up next! :)

@shaddockh shaddockh force-pushed the shaddockh:TSH-ATOMIC-TYPESCRIPT branch from bd9c056 to 4570255 Jan 31, 2016

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Feb 22, 2016

These latest changes move the typescript language service into the web view.
Server Side Changes

  • Made a copy of a lot of the editor event interfaces into a new EditorWork.d.ts so that they could be shared with the Webview code and optionally other extensions
  • Modified when the host notifies the web view to load code until after the webview indicates that all of the client side code has loaded. There was a race condition happening and the host was trying to call into a routine that didn't actually exist yet.
  • Removed the typescript language services out of the host and into the webview
  • Left the host typescript extension to provide the following
    • auto-delete .js file if the corresponding .ts file is deleted
    • build out a tsconfig.atomic in the root of the resources directory if it is detected that there are typescript files in the project
    • auto-rename .js file if the .ts file is renamed

Client Side Changes

  • converted to the non-bundle version of SystemJS. This will allow for loading extensions on demand if necessary and keeps from having to load the entire client in at once
  • created new client side extension services model for hooking up extensions on the client side. The idea being that language specific features could be extracted out into dedicated extensions and not be interleaved with the core code. Ideally the core code would be agnostic and any specifics would be handled by a robust extension model.
  • added new Javascript extension and Typescript extension.

Javascript Extension

  • hooks into the editor config hook to set up the editor mode with the javascript mode

Typescript Extension

  • hooks into the editor config hook to set up the editor mode with the Typescript mode
  • Provides auto-completion of typescript. It will also show parameter completion for completions in the popup. (It does not, however show parameter completion if you just hover over a word in the editor. This should probably be researched)
  • requests tsconfig.atomic from the server and then pulls down all the source files into the web view to be able to reference during completion

TODO

  • Determine if there is a way to do parameter completion popups on hover over the text
  • Move the routine that pulls down all of the files and provides completion into a shared web worker so that 1) it runs on a background thread, and 2) it can be shared by all editor instances
  • Implement compilation services in the web worker and update the C++ to be able to save the compiled files (Most of this is already in place)
  • Implement a way to expose any compilation errors to the editor
  • Fine tuning
@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Feb 23, 2016

Wow, this is really impressive and GREAT leveraging on the power of the Chromium WebView.

It would be good to figure out the shortest path to getting it on master (following up with the TODO's). Do you have thoughts on what strictly needs to go in to achieve this? For example, I'd be happy with having the autocomplete for TS working initially and following up with compilation and error reporting.

As master moves forward it may become more work to sync with it. I think this is relatively localized though, you may want to rebase once a couple imminent PR's land.

I am also wondering if you are running into any limitations on the native API that you need me to look at?

Awesome work 👍

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Feb 23, 2016

That sounds like a really good idea and it would be nice to break this up a little bit. I may have hit a roadblock last night trying to get a SharedWorker running inside the web view. What this should allow is for multiple editor instances to communicate with a single instance of the Typescript language service. This would ensure that only one copy of the compiled version of each file is in memory instead of a copy per editor instance. I didn't have much success so far, so that may need to be something that's handled via IPC with the host.

Before the completions should drop into master, they really need to run in a worker process instead of on the main webview UI thread. I did have some success getting a worker process to spawn, but it still needs more work and I need to migrate the autocomplete logic into it. Once that looks good, I would say that this PR could merge in and the remaining TODO items broken out into separate issues. I'd like to see how far I get over the weekend and see if it's in a stable enough point to cut over.

@shaddockh shaddockh force-pushed the shaddockh:TSH-ATOMIC-TYPESCRIPT branch from 28216f6 to 0f7509c Feb 27, 2016

@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Feb 27, 2016

Latest changes migrated the autocomplete and doc tooltip logic into a shared web worker so that:

  • It runs in a background thread and doesn't slow down the ui
  • the same in-memory compiled files can be referenced by multiple tabs instead of each tab containing it's own copy

Additionally, the chrome dev tools are turned on and can be accessed by pointing chrome to:
http://localhost:3335

for just having auto-complete functionality (not compilation functionality), the remaining things still need to be done:

  • need to have the host notify the editor when files are renamed, deleted, or updated
  • need to have the host notify the editor when the project is closed so that the language service can shut down
  • need to be able to create new typescript files in the editor. Right now, when you create a new file, it automatically gives it a .js extension.
@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Feb 27, 2016

I may need some help getting those events sent to the webview editor. Basically, the same editor events that get transmitted to the host typescript code need to be transmitted to the webview so that the webview can handle those situations.

Also, would it be possible for there to be a mechanism to allow the host editor code to communicate with the webview? ie, some way for either the webview to request information from the editor code side or the editor code to send information/events to the webview code?

finally, I'm seeing some performance issues with having "live" autocomplete on. It might be better to have autocomplete only triggered when requests (ctrl/cmd space). I think this might be something that could be exposed in an options dialog, but until then it might be a good idea to leave it turned off. I was running into issue where it seemed to cause keys to be lost.

For the issue where only .js files are getting created, the right way to do it would be to extend out that new file dialog to have a language dropdown just like the project dialog does. I can look at that if you like or that can be deferred to another issue and for now only append a .js if a file extension is not specified in that dialog.

shaddockh added some commits Jan 16, 2016

initial cut at allowing typescript to be compiled by the editor and h…
…aving the ability to hook additional services into editor events
small update to build common to make sure that the js modules directo…
…ry gets copied over to the build. May need to find a cleaner way
updated build script to copy typescript.js from the node_modules dire…
…ctory and place it in the editor/modules directory

shaddockh added some commits Feb 20, 2016

Enable remote debugging of the web view with the chrome dev tools. Po…
…int your browser to http://localhost:3335 to use
migrated the autocomplete functionality for typescript into a shared …
…web worker so that each tab can share the same instance of the language service. It also runs on a background thread so it performs better
staging the webview extension code so that it will be ready to receiv…
…e a project unloaded event message, a delete resource event message, and a rename resource event message. Also did some cleanup to reduce duplicate code
as a stopgap measure, if a file extension is not provided for creatin…
…g a new component or script, it will default to .js, but if one is provided, then it will use that.

@shaddockh shaddockh force-pushed the shaddockh:TSH-ATOMIC-TYPESCRIPT branch from c5cba9e to f5d1e6c Mar 18, 2016

shaddockh added some commits Mar 31, 2016

Added delete, rename, and project unloaded events to the native side …
…so it can route these events to the web view
Work on routing delete, rename, and project unloaded events to the we…
…b view so that the typescript language service can keep it's internal representation of the project in sync
@shaddockh

This comment has been minimized.

Copy link
Collaborator Author

shaddockh commented Mar 31, 2016

I've implemented the logic that allows the editor to route rename, delete, and project closed events through the native side and down into the web view. This was the last piece that was required to try and keep the web view representation of the project in sync with the host. I did this by creating new native events, but apparently there is a more direct way to communicate between the web view and the host script?

This PR is pretty huge at this point, so it's probably going to be a good idea to lock down the features and make sure everything looks good. Future PRs could implement the actual Typescript compilation piece and error checking.


// copy lib.core.d.ts into the tool data directory
fs.mkdirsSync("./Artifacts/Build/Resources/EditorData/AtomicEditor/EditorScripts/AtomicEditor/TypeScriptSupport");
fs.copySync("./Build/node_modules/typescript/lib/lib.core.d.ts","./Data/AtomicEditor/TypeScriptSupport/lib.core.d.ts")
complete();

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

Looks like I will need to update the CI scripts once landed (also testing Windows), making a note

log(message?: any, ...optionalParams: any[]): void;
}

declare var console:Console;

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

Cool add :)

/**
* Generic registry for storing Editor Extension Services
*/
class ServiceRegistry<T extends Editor.Extensions.EditorService> implements Editor.Extensions.ServiceRegistry<T> {

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

This may be the first use of a generic in the editor scripts, kudos!

export default serviceLocator;

// Load up all the internal services
serviceLocator.loadService(new TypescriptLanguageExtension());

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

The service stuff is such an awesome bonus PR feature! It will be interesting to knock it around with project defined services!

This comment has been minimized.

@shaddockh

shaddockh Apr 1, 2016

Author Collaborator

This is just the foundational piece. In order to get per-project extensions to load in, the service locator will need to be extended to scan the project directory for extensions, dynamically require them in, instantiate them, and add them to the service registry. That's something I'll be looking at after this PR lands.

*/
delete(ev: Editor.EditorEvents.DeleteResourceEvent) {
if (this.isValidFiletype(ev.path)) {
console.log(`${this.name}: received a delete resource event`);

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

We'll probably want a logging JS API here which can be set to various log levels, like native side. console.log's could get spammy

This comment has been minimized.

@shaddockh

shaddockh Apr 1, 2016

Author Collaborator

that's a really good idea. There may be a Log4JS or something similar that could be utilized. I'll write up a ticket for that as something to look into in the future.

newPath: jsFileNew,
newName: ev.newName,
asset: jsFileAsset
};

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

Still happy with JS Object <-> VariantMap event pattern, looking good

@@ -1 +1 @@
Subproject commit a74fdafc2f42f4349d777e1a1fb20f63e848da23
Subproject commit 0193bbffe0a032649014cf032bcea08ec58eed7f

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

It looks like the submodule got updated somehow here, a74fdafc2f42f4349d777e1a1fb20f63e848da23 is the correct hash.

This comment has been minimized.

@shaddockh

shaddockh Apr 1, 2016

Author Collaborator

I can't seem to find this file anywhere in the filesystem. Where is it located?

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Apr 1, 2016

Contributor

I made a PR on your fork which should fix it when merged (and automatically update here), I think this is stored in the git index

// shave off atomic://resources/
url_ = url_.SubstringUTF8(19);
//shave off the atomic:// part
url_ = url_.SubstringUTF8(9);

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Mar 31, 2016

Contributor

Nice spelunking and good change!

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Mar 31, 2016

This is a really awesome PR with crisp coding standards. The new service architecture elevates the current editor scripts and are a great example of better TS usage in general. Thanks for all the hard work and I am really looking forward to testing this out in a little bit on Mac/Windows. There is one issue with the submodule having been accidentally changed, otherwise very solid! 👍

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Mar 31, 2016

I built the PR on OSX and Windows, worked great on both! 🍔

It seems there is some lag introduced by the having the automatic autocomplete as ya mentioned, especially on Windows. I am not sure if there is something we can do there. I don't think we're pegging a CPU in the V8 process. It would be interesting if there was an example of TS autocomplete on the web somewhere using Ace (a tall order?), we could see if it had similar performance characteristics.

In any event, ready to land it once the submodule commit is fixed. Fantastic stuff!

JoshEngebretson and others added some commits Apr 1, 2016

@JoshEngebretson

This comment has been minimized.

Copy link
Contributor

JoshEngebretson commented Apr 1, 2016

Aw-hoooo-gah, aw-hoooo-gah, batten down the hatches, this TypeScript PR is hitting master.... NOW! :)

I'll also kick off a build to make sure nothing funky happened on merge (since there are a few changes on master now represented in the branch)

Awesome work, THANKS again!

@JoshEngebretson JoshEngebretson merged commit 518fef5 into AtomicGameEngine:master Apr 1, 2016

1 check passed

default Build finished. No test results found.
Details
// LICENSE: Atomic Game Engine Editor and Tools EULA
// Please see LICENSE_ATOMIC_EDITOR_AND_TOOLS.md in repository root for
// license information: https://github.com/AtomicGameEngine/AtomicGameEngine
//

This comment has been minimized.

@JoshEngebretson

JoshEngebretson Apr 1, 2016

Contributor

Oops, I missed the licensing block in the original PR. Would you mind switching these to the MIT block in a followup?

For example: https://github.com/AtomicGameEngine/AtomicGameEngine/blob/master/Script/AtomicEditor/main.ts#L1

This comment has been minimized.

@shaddockh

shaddockh Apr 1, 2016

Author Collaborator

sure thing!

@shaddockh shaddockh deleted the shaddockh:TSH-ATOMIC-TYPESCRIPT branch Apr 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment