Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Porting tern analysis of JS code to Node. #10304

Open
2 tasks done
core-ai-bot opened this issue Aug 30, 2021 · 28 comments
Open
2 tasks done

[CLOSED] Porting tern analysis of JS code to Node. #10304

core-ai-bot opened this issue Aug 30, 2021 · 28 comments

Comments

@core-ai-bot
Copy link
Member

Issue by swmitra
Monday Nov 23, 2015 at 09:15 GMT
Originally opened as adobe/brackets#11948

This PR is created to port our js code analysis logic on node for better scalability . This will improve the overall performance and leverage addition of new features like -

  1. In place rename of variable with scoping [ Separate PR will enable this soon ]
  2. Find reference with scoping [ Separate PR will enable this soon ]
  3. Call Hierarchy [ Separate PR will enable this soon ]

and other related features.

TODO

  • Fetching of doc content from node domain instead of posting request back to main thread
  • Unit test update

swmitra included the following code: https://github.com/adobe/brackets/pull/11948/commits
@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Nov 23, 2015 at 09:17 GMT

Tagging@nethip@abose@zaggino@ryanstewart@petetnt

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Nov 24, 2015 at 09:21 GMT

@abose WIP! Removed the old reference now.

@core-ai-bot
Copy link
Member Author

Comment by abose
Tuesday Nov 24, 2015 at 09:23 GMT

Would the path change affect the extensions?

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Monday Nov 30, 2015 at 07:30 GMT

@swmitra Have you done any initial performance benchmarks against the previous implementation yet 📊?

I can try and review this ASAP, been a bit busy as of late

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Monday Nov 30, 2015 at 16:46 GMT

@swmitra Do you think it would be possible / easier to add the reading of a .tern-config file (even in a future PR)?

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Tuesday Dec 01, 2015 at 07:53 GMT

Sorry for late reply.

@petetnt Yes I tried to benchmark the performance to have the gain quantified (Will post a detailed result once the implementation is complete).
Any help on performance data with and without this PR is welcome!
For small and medium JS files , there is marginal gain which may not be even perceptible. The benefits of porting this analysis to tern node has 2 main agendas -

  • File content extraction can be done from node domain itself. I am currently working on this. This will allow us to perform the complete analysis and preparation for the analysis outside main thread as we don't have to post back messages to the main thread asking for file content.
  • We do have a bottleneck in terms of performance when we are asking for hints in htmlmixed content and the html file is large enough ( may be 10k+ lines of code ). The typing response gets severely affected as the main thread gets hogged to extract JS text from the html file. I am trying to use a CM runmode on node to offload this to node domain. This will improve the typing performance of the main editor significantly.

@ficristo Yes , it's a good idea to have a package.json and resolve the dependencies as node modules. Also we can introduce a tern config file. I will take both this activities outside this PR and do the changes under a separate PR soon after this.

@core-ai-bot
Copy link
Member Author

Comment by yacut
Sunday Jun 19, 2016 at 20:49 GMT

Would be possible to enable nodejs plugin with this feature?

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Jul 21, 2016 at 08:09 GMT

@swmitra What is the status of this?

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Thursday Jul 28, 2016 at 03:39 GMT

I am really sorry@ficristo for late reply.
This is not yet ready for delivery. Currently I am help with a lot of work. I will try to look at this next week.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Aug 11, 2016 at 19:24 GMT

@swmitra could you update this to latest master?

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Thursday Jan 19, 2017 at 13:25 GMT

@zaggino Thanks a lot for the quick review. I will update the PR with the requested changes. As this PR was raised before the dependency handling change in Brackets, gitmodule change is still visible in change list. I will make that even with master.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Friday Jan 27, 2017 at 22:10 GMT

This should be absolute must for 1.9@swmitra , I believe that it will fix issues some people have with Brackets crashing on them when viewing certain files. cc@ficristo

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Sunday Jan 29, 2017 at 03:31 GMT

Sure@zaggino. I will be on it.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Thursday Feb 02, 2017 at 08:03 GMT

@zaggino I have removed the redundant module definitions and the post install script as well. Please have a look at the PR. Meanwhile I will do some unit testing around inference data clearance on project change ( that's the only bug remaining now).

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Friday Feb 17, 2017 at 10:08 GMT

While I believe this is a must for us, I think this will break js-code-hints the in-browser approach.
Maybe there is something we could do to be less problematic for them?
/cc@humphd

@core-ai-bot
Copy link
Member Author

Comment by humphd
Friday Feb 17, 2017 at 15:59 GMT

@ficristo thanks for alerting me to this (and if you see other things that you think might break us, I'd appreciate being cc'ed again). If we have to, we can just fork the existing extension and use that, while you move forward with a node-based approach. cc@ gideonthomas

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Feb 21, 2017 at 21:29 GMT

@swmitra I believe last comment here: adobe/brackets#12617 (comment) proves that this is an absolute must to merge for 1.9

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Mar 13, 2017 at 06:19 GMT

@zaggino@ficristo@petetnt Can you guys please have a look at the code now? If there are any breaking changes or adverse effect on brackets, we can move this to 1.10 milestone and unblock the 1.9 release.
We are already in the process of 1.9 release activities and having a release of Brackets is an absolute must as it's over four months we did the last release. Our goal is to have a release every 2 months from now on.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Wednesday May 31, 2017 at 06:30 GMT

@zaggino@petetnt Can you guys please have a look at this?
This is a pending feature for 1.10. I have resolved all the outstanding issues/bugs and JS hinting, jump to def all are working fine.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Wednesday May 31, 2017 at 07:58 GMT

@swmitra running the branch right now at work, I can give some feedback later.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Friday Jun 02, 2017 at 07:20 GMT

Been using this branch for couple of days without issues except CMD+O (quick open) won't open files that are already open in the working set, which is a regression from the current behaviour. Haven't had the time to dive in whats up with that, but FYI 👍

Unrelated

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Monday Jun 19, 2017 at 13:21 GMT

Pushing this to next release. We will merge this the moment we release 1.10 and do the first pre-release with this PR. It's breaking a few unit tests and have to resolve them before we merge.

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Sunday Jul 09, 2017 at 12:46 GMT

@swmitra Based on your last comment, this is now ready to be merged.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Sunday Jul 09, 2017 at 13:35 GMT

Yes@MarcelGerber 😄 . If we need a build refresh for the reported dll issue, we would be in trouble in case this PR is merged. I would wait for a couple of days to discuss any possibility of build refresh and then merge this PR for the first pre release.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Wednesday Jul 12, 2017 at 09:10 GMT

@MarcelGerber This PR can't be merged right now 😞. There are 2 major issues I found today while testing -

TOFIX

  • Require module hints have stopped working
  • If node restarts for any failure, re initialization of Tern should happen again

While I am working on these 2 issues, if you can spot any problem or a probable cause for this, please let me know. These should be fixed in a day or two.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Wednesday Jul 12, 2017 at 16:34 GMT

Acorn and Tern have been upgraded to latest version 5.1.1 and 0.21.0 respectively. Haven't noticed any problem in real use or test suite except the ones mentioned before.

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Thursday Jul 13, 2017 at 13:55 GMT

@MarcelGerber@petetnt Just the final bit remaining. Need to re-initialize Tern when node gets restarted after a crash. Can we merge this PR now? We can get some time for testing all the features then.

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Thursday Jul 13, 2017 at 14:45 GMT

LGTM with a follow up PR on the restart-after-crash scenario

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot