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

JS Code Hints Crash Preventer #8155

Merged
merged 4 commits into from
Jun 19, 2014
Merged

JS Code Hints Crash Preventer #8155

merged 4 commits into from
Jun 19, 2014

Conversation

redmunds
Copy link
Contributor

  1. Update Tern
  2. Implement timeout

cc @dangoor

@redmunds
Copy link
Contributor Author

Note: The "should list matching property names" unit test is failing. The updated Tern is returning 2 more items to hint list, so test is breaking.

I can't figure out what it's supposed to test since the cursor position { line: 12, ch: 10 } is non-existent in the test file (i.e. that's an empty line). Also, the "param" string is removed from file at start of test, so I'm not sure how expected result is ["paramB1", "paramB2"].

@@ -154,7 +180,7 @@ var config = {};
query.expandWordForward = false;
query.lineCharPositions = true;

var request = {query: query, files: [], offset: offset};
var request = {query: query, files: [], offset: offset, timeout: 5000};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should make timeout a preference.

@dangoor dangoor self-assigned this Jun 18, 2014
@dangoor
Copy link
Contributor

dangoor commented Jun 18, 2014

This looks encouraging. I'm going to do some testing with code that has previously been problematic.

@dangoor
Copy link
Contributor

dangoor commented Jun 18, 2014

I've tried this on a couple of problem projects (one with Enchant.js and one with Ionic, after removing the default exclusion for Ionic). In both cases, the timeout caught problem files and Brackets continued to behave well afterwards. A huge improvement!

In the case of the Ionic project, it was, mysteriously, timing out on the file I was editing... which is not something I would expect. Probably worth digging into that a bit more.

Regarding the "should list matching property names", what the test is doing is "typing" param on that empty 13th line of file1.js. If you do that manually in Brackets, you'll see that in release 40 you get two names and with this change you get four. I'm guessing that the defs/browser.json file added HTMLParamElement and HTMLParagraphElement in this update to Tern, which is why we have four names instead of two. The new result is fine, so we could just change the test to include the four names we expect.

The reason I'm thinking we'd want to save these as new exclusions is so that we don't keep burning through CPU by continuing to try to process these files. Also, it might be nice for the user to know that they may not get hints as expected... and finally, it might be good if we can ship some of these cases off to Marijn and see if he can fix the root causes in Tern.

All of that said, I think we should ship this in 41, even just logging to the console. This looks to be really effective at stopping the pain.

@redmunds
Copy link
Contributor Author

@dangoor Fixed unit test. Not sure why Interfaces are being displayed in that context, so just added a char to what's "typed" to narrow down list. Also, fixed cursor positions to be accurate.

@redmunds
Copy link
Contributor Author

@dangoor

In the case of the Ionic project, it was, mysteriously, timing out on the file I was editing…

Could it be that the timeout is on the correct file, but the filename displayed in the message is incorrect? If not, maybe increasing timeout will mitigate that for now?

@dangoor
Copy link
Contributor

dangoor commented Jun 19, 2014

@redmunds It looks like HTMLParamElement is a legit, though likely unused, hint (from the Chrome console):

> HTMLParamElement
function HTMLParamElement() { [native code] }

The test change looks fine.

I'll do some more testing on the Ionic project. I should have mentioned that it hit the timeout on some other file first (a minified Angular file, IIRC) and then started timing out on the file I was editing. It's possible that there's some other behavior difference between the new Tern version and the old. Regardless, I still expect to land this soon.

@RaymondLim
Copy link
Contributor

@redmunds Are there any reasons not to update acorn to the latest?

@redmunds
Copy link
Contributor Author

@RaymondLim Not that I know of, but since this PR fixes a crash, I think updating acorn should be done in a separate PR so as to not prevent this one from getting in Release 0.41.

@dangoor
Copy link
Contributor

dangoor commented Jun 19, 2014

Good suggestion @RaymondLim. Tern itself relies on Acorn (and they're both by Marijn), so it's usually a good idea to update them together.

@dangoor
Copy link
Contributor

dangoor commented Jun 19, 2014

Everything checks out. Merging!

dangoor added a commit that referenced this pull request Jun 19, 2014
@dangoor dangoor merged commit 0d3603e into master Jun 19, 2014
@dangoor dangoor deleted the randy/update-tern branch June 19, 2014 19:22
@maxwowpow
Copy link

Hello, no intellisense in all files in my project. How should I troubleshoot?

  • with a new project it works.
  • plugins disabled - same result - no code hinting.
  • local settings removed - no changes.
  • 0.40 worked.

-- project cannot be uploaded or presented for investigation

200Mb Ram... soon will be the same as bloated visual studio with 500mb.
Lags on Plugins initialization and files of 1000 lines of code. sigh.

@redmunds
Copy link
Contributor Author

@maxgrass Start with Debug > Show Developer Tools to see if there are any errors in the console. Note that this is a pull request, so if you have any further questions, please create a new issue.

@peterflynn
Copy link
Member

@maxgrass Brackets shouldn't be slow on a file that's just 1000 lines long. Please file a bug on that with as much detail as possible (what else is in your project, does it happen in all your projects or just one, etc.).

Please also file a bug about the code hints issue. Lack of JS code hints could be caused by any number of things. Are you missing hints for code that's in the same file as where you're typing? Or in a different file? If a different file, where is the code located related to the file you're typing in? Is it referenced by the file you're typing in (e.g. via RequireJS)? Please include details like that when you file the bug.

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants