Adding content assist support for Closure

[peaches]

It doesn't look like there's another mechanism to add support for content assist other than adding files directly into server code. As a result, this PR is somewhat invasive.

The way goog.require and goog.provide works is so that when the code is put through the closure compiler, it all gets stripped away and the files are all located in the proper order. In development mode, you can use the DepsWriter to generate an intermediate file that builds a dependency tree. This PR relies on that intermediate file to locate the correct files (in the resolver) for content assist. Here are the added functionalities:

• Support for .scripted configuration to point to the deps file: { "closure" : { "deps" : "relative/path/to/deps.js" } }
• A deps file loader (regex) to map modules/constructors to file
• Adds a new kind of module, "closure".
• Treats the "closure" type as global (namespaced of course)

[aeisenberg]

Thanks for the pull request. I'll have a look at this on Monday.

Does your pull request include any tests?

The tests are located here: https://github.com/scripted-editor/scripted/tree/master/tests/client/esprima summaryBuildingTests.js and contentAssistDependencyTests.js are what are most relevant here.

[peaches]

Nope, that was one of the things I wanted to ask one of you guys about. Since my resolver is dependent on a generated file, what is the best way for me to write test for this?

It looks like I can get started on writing some of the tests with a MockIndexer. In addition, it looks like the Travis build failed with Unable to access network: fail. I'm not sure what that's all about, so I'll take a look into that either today or tomorrow.

[aeisenberg]

Well, if the file is generated by closure compiler, part of the mock indexer for these tests would just be to supply this generated file. Or is there a complication that I'm missing?

I'm not sure what that error message means. You can try running the tests on the command line npm test or in the browser http://localhost:7261/clientTests. If all the tests pass this way, then I am less concerned about the travis failure.

On Sun, Jun 2, 2013 at 6:24 PM, akngo notifications@github.com wrote:

Nope, that was one of the things I wanted to ask one of you guys about. Since my resolver is dependent on a generated file, what is the best way for me to write test for this?

It looks like I can get started on writing some of the tests with a MockIndexer. In addition, it looks like the Travis build failed with Unable to access network: fail. I'm not sure what that's all about, so I'll take a look into that either today or tomorrow.

— Reply to this email directly or view it on GitHubhttps://github.com/scripted-editor/scripted/pull/280#issuecomment-18818034 .

[peaches]

My main concern was the server side portion of the dependency resolution. File location and "module" names are not at all related to one another. I'll take a look at the server side tests to see if there's something I can imitate.

As far as client side testing goes, I added c2a150f476ecf997bcc2db6a9f271b831ae833fc, which has limited testing since closure is just relying on the global namespace, which are tested via the global tests. Let me know if you have anymore insights on what other things I could test on.

The network problem seemed like a transient network issue, tests were passing locally.

[aeisenberg]

Comments:

1. please run git rebase -i to squash everything into a single commit. This helps with checking for changes.
2. The client side pieces look good to me, except for the minor issue below.
3. In checkForClosureModule, there is a potential type error. body[0].expression.callee may be null if the code is malformed. So, add a check for that.
4. I'll ask @kdvolder to comment on the server side pieces, since this is the part that he wrote.
5. I'd like to see some server side tests as well. Again, @kdvolder can give you some pointers on how to do this.
6. Lastly, and the most fun part. You will need to sign a CLA before we can merge.

Thanks again for your effort.

[peaches]

1. Squashed into a single commit, is it customary to do this with each subsequent commits or is it okay just before the merge?
2. Fixed callee issue
3. I can't find this project at https://support.springsource.com/spring_committer_signup, and should I put your name for project lead?

As for server side tests, I will get to it Wednesday, I got stuck trying to add something else today, since I'd like to have it for my current project.

[peaches]

Added server side tests for the deps file loader, the resolver, and the reference finder. Is there an electronic CLA for scripted? Or do you recommend me going with the pdf printing option?

[aeisenberg]

Don't go for the printing option. It should be available online very soon.

On Thu, Jun 6, 2013 at 3:00 AM, akngo notifications@github.com wrote:

Added server side tests for the deps file loader, the resolver, and the reference finder. Is there an electronic CLA for scripted? Or do you recommend me going with the pdf printing option?

— Reply to this email directly or view it on GitHubhttps://github.com/scripted-editor/scripted/pull/280#issuecomment-19035502 .

[peaches]

Sorry for the late response, I'm preparing for finals and it's taking up all of my time. I'll respond to these PRs after finals.

[kdvolder]

No problem. There's no rush. Let me know if you have questions about the server side code.

[cm325]

Any update on when this will be able?

[peaches]

Closing this as I no longer have the time to follow up on this.