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.

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

remove cycles in objects that are moving between server and browser #233

Merged
merged 11 commits into from May 23, 2019

Conversation

2 participants
@ariel-bentu
Copy link
Contributor

commented May 19, 2019

makes them smaller and work in Theia

It reduces size drastically and also makes it work better in Theia

Ariel Bentolila

@project-bot project-bot bot added this to In progress in Kanban May 19, 2019

@codecov

This comment has been minimized.

Copy link

commented May 19, 2019

Codecov Report

Merging #233 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #233   +/-   ##
======================================
  Coverage    3.51%   3.51%           
======================================
  Files          85      85           
  Lines        2960    2960           
  Branches      605     606    +1     
======================================
  Hits          104     104           
  Misses       2843    2843           
  Partials       13      13
Impacted Files Coverage Δ
...ckages/plugins/bookmarks-manager/explorer/index.ts 0% <0%> (ø) ⬆️
...kages/plugins/connection-manager/explorer/index.ts 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d07c8b...a355b17. Read the comment docs.

@mtxr

This comment has been minimized.

Copy link
Owner

commented May 20, 2019

Cant accept this 'as-is'. It's breaking on VSCode. Is there a env variable that identifies we are running on Theia?

This way we could check to use 'VSCode API' or 'Theia API'.

Any ideas or suggestions?

@mtxr
Copy link
Owner

left a comment

Take a look at the previous comments

@ariel-bentu

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Ariel Bentolila and others added some commits May 21, 2019

@mtxr

This comment has been minimized.

Copy link
Owner

commented May 22, 2019

I'll reach you in the end of the day if the tests, ok?

It's small stuff. What you did is right and I see performance gain on this

@@ -245,12 +245,17 @@ export class ConnectionExplorer implements TreeDataProvider<SidebarTreeItem> {
}

public setActiveConnection(c: ConnectionInterface) {
Object.values(this.tree).forEach(item => {

This comment has been minimized.

Copy link
@mtxr

mtxr May 23, 2019

Owner

Ok, this is where I saw that was braking.

I should we move this code to a few lines below?

I didn't get the point of that.

Anyway I'm trying to reproduce to see the stack trace, but I don't remember exactly what I did that broke it :( lol

This comment has been minimized.

Copy link
@mtxr

mtxr May 23, 2019

Owner

Nvm. It was breaking because of the shadow vairable item. renaming fixes! Thanks @ariel-bentu!

I can see the performance enhancement in VSCode as well!

mtxr added some commits May 23, 2019

@mtxr

mtxr approved these changes May 23, 2019

@@ -245,12 +245,17 @@ export class ConnectionExplorer implements TreeDataProvider<SidebarTreeItem> {
}

public setActiveConnection(c: ConnectionInterface) {
Object.values(this.tree).forEach(item => {

This comment has been minimized.

Copy link
@mtxr

mtxr May 23, 2019

Owner

Nvm. It was breaking because of the shadow vairable item. renaming fixes! Thanks @ariel-bentu!

I can see the performance enhancement in VSCode as well!

@mtxr mtxr merged commit 05ebdc6 into mtxr:master May 23, 2019

3 checks passed

Mergeable Mergeable Run has been Completed!
Details
Travis CI - Pull Request Build Passed
Details
codeclimate 2 fixed issues
Details

Kanban automation moved this from In progress to To be released May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.