Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Switch js code hinting to be powered by tern. #3439

Merged
merged 96 commits into from
Apr 18, 2013

Conversation

eztierney
Copy link
Contributor

This pull request switches the js code hinting to use tern (http://ternjs.net). The hints are now based on tern's type inference library. When a type can be inferred, the hints are based on that type, and when a type can't be inferrred then the hinting falls back to guessing. In addition to type inference, types can also be determined based on jsdoc annotations, and an understanding of what types are exposed from requirejs modules.

This also adds hinting for function types (can display the types of the arguments of the function being called), and support for a jump to definition feature.

For more details, see
https://github.com/adobe-research/brackets/wiki/JavaScript-Code-Intelligence-Investigation

Erik Tierney and others added 30 commits March 18, 2013 13:57
Currently only works in 1 file
tern does not yet run in a web worker
Code Hinting plugin now uses tern hints for property completions, and falls back to its own heuristics if tern does not have any hints.
…ing the completions.

This means there is now a delay on the first request for hints.
Add some logging when we fail to read one of the json files
…periments

Conflicts:
	src/extensions/default/JavaScriptCodeHints/ScopeManager.js
Had to add requirejs to the extension to make everything work in a web-worker.
Added tern-worker.js, and modified ScopeManager to post messages to the worker to find completions.
Use tern to sort ids by depth, remove scope info
return (/[0-9a-z_.\$]/i).test(key) ||
(key.indexOf(SINGLE_QUOTE) === 0) ||
(key.indexOf(DOUBLE_QUOTE) === 0);
return (/[0-9a-z_\$]/i).test(key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obviously not a regression from current behavior, but I do wonder if this code is going to fail on valid identifiers. Ahh, I see. It does fail some in that if I have a variable called façade, if I type faç the hinting stops until I hit the a. Anyhow, it's a minor point and nothing to fix now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thats obviously not going to work.

I filed adobe-research#38 - we probably just want to ask the token if it is an identifier, since the token should have the correct info.

@dloverin
Copy link

We are using multiFieldSort() with matchGoodness being the highest priority. I would expect a string that matches a prefix in the hints would have a higher matchGoodness score than one that just matches camel case. If that was the case then "str" would be sorted to the top and there wouldn't be an issue.

@dangoor
Copy link
Contributor

dangoor commented Apr 18, 2013

@dloverin I think I agree with you that a prefix match deserves a boost big enough to put it ahead of a camelCase match. But, that still doesn't completely fix this case. "st" would then match "str", "String", "stringMatch", "StringMatcher", and I would argue that the order should be "str", "stringMatch", "StringMatcher".

Not a blocker, certainly, and I can look into resolving #3411 during this sprint (should not be a lot of work).

@dloverin
Copy link

@dangoor I agree "String" should be moved below "str", "strMatch", "StringMatcher". If all four strings have the same matchGoodness score then the issue should be fixable in the hinter.

* Read in the json files that have type information for the builtins, dom,etc
*/
function initTernEnv() {
var path = module.uri.substring(0, module.uri.lastIndexOf("/") + 1) + "thirdparty/tern/defs/";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: may be cleaner to use ExtensionUtils.getModulePath for this construct which appears in multiple places.

*
* @param {string} path - the target path name
* @return {Function} - the comparator function
* @param {string} origin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should add a @return

@dangoor
Copy link
Contributor

dangoor commented Apr 18, 2013

Review complete.

The results with the new type inference and other features are awesome. The code looks good. My comments were all at the "nit" sort of level. Minor suggestions for improvement. I can file some separate issues for those. My intention is to merge this as-is as soon as we figure out why Travis is complaining.

dangoor added a commit that referenced this pull request Apr 18, 2013
Switch js code hinting to be powered by tern.
@dangoor dangoor merged commit 372ce5a into adobe:master Apr 18, 2013
@eztierney
Copy link
Contributor Author

awesome, thanks!
I should have all the cleanup done shortly

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants