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

Fix issues with extension sync #907

Merged
merged 2 commits into from
Jun 11, 2019
Merged

Fix issues with extension sync #907

merged 2 commits into from
Jun 11, 2019

Conversation

auxves
Copy link
Contributor

@auxves auxves commented Jun 11, 2019

Short description of what this resolves:

This PR fixes issues that prevented extensions from being synced properly.

Changes proposed in this pull request:

  • Properly throw an error when one occurs
  • Scan for missing extensions correctly

Checklist:

@shanalikhan shanalikhan merged commit cbffb6c into shanalikhan:v3.3.0 Jun 11, 2019
@shanalikhan shanalikhan added this to the v3.3.0 milestone Jun 11, 2019
@shanalikhan
Copy link
Owner

We need to sync the map function by using async/await in the following line.
https://github.com/shanalikhan/code-settings-sync/blob/v3.3.0/src/service/pluginService.ts#L225

Can you improve this and send me PR.
Delete extension seems to working fine, i will test more.

The current output:

[x] - EXTENSION: beautify - INSTALLING

[x] - EXTENSION: debugger-for-chrome - INSTALLING

[x] - EXTENSION: team - INSTALLING
    - EXTENSION: debugger-for-chrome - INSTALLED.
      1 OF 3 INSTALLED

    - EXTENSION: team - INSTALLED.
      2 OF 3 INSTALLED

    - EXTENSION: beautify - INSTALLED.
      3 OF 3 INSTALLED

``

@auxves
Copy link
Contributor Author

auxves commented Jun 14, 2019

@shanalikhan The problem with syncing the two is that Promise.all executes the installations in parallel. The only way I could think of to resolve this would be to use blocking code, which is not recommended for a VSCode extension because it can freeze the extension host. What I suggest we do is just remove the log stating that an extension is installing and only leaving the one that says that it was installed. What do you think?

@shanalikhan
Copy link
Owner

shanalikhan commented Jun 14, 2019

just remove the log stating that an extension is installing

For the user point of view, we must show which extension is installing and installed completely.

it can freeze the extension host

Yes , we should not use that.
What about we can use simple for loop instead of map and other stuff. Explore other ways.
You can look into the previous code in master branch, how in CLI i'm holding the next installation CLI command that waits for previous command to complete.

But we need to show what is going to install and what has been installed.

@auxves
Copy link
Contributor Author

auxves commented Jun 14, 2019

What about we can use simple for loop instead of map and other stuff.

That is an example of blocking code. For loops prevent other code from running until it is finished, and extension installation takes a long time.

Edit: I guess we will have to use for loops after all. I couldn't find a suitable replacement.

@auxves
Copy link
Contributor Author

auxves commented Jun 14, 2019

@shanalikhan Here's a PR that will sync the INSTALLING and INSTALLED messages: #910

@auxves auxves deleted the fix-unable-to-remove-some-extensions branch June 19, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants