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

cli: replace request package #7677

Merged
merged 1 commit into from
Apr 27, 2020
Merged

cli: replace request package #7677

merged 1 commit into from
Apr 27, 2020

Conversation

paul-marechal
Copy link
Member

requestretry looks very bogus and forced us to put buffering in place,
which slowed down the whole script drastically.

This commit replaces requestretry by node-fetch which better handles
node streams.

How to test

See #7662 (comment)

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added enhancement issues that are enhancements to current functionality - nice to haves theia-cli issues related to the theia-cli labels Apr 27, 2020
`requestretry` looks very bogus and forced us to put buffering in place,
which slowed down the whole script drastically.

This commit replaces `requestretry` by `node-fetch` which better handles
node streams.

Signed-off-by: Paul Maréchal <[email protected]>
@lmcbout
Copy link
Contributor

lmcbout commented Apr 27, 2020

@marechal-p . I added some java plugins, tested and it is fast for redhat-java as well
TEST:
rm -rf plugins
time yarn download:plugins

One thing the vscode-builtin-node-debug is still failing

x vscode-builtin-node-debug: failed to download with: 404 Not Found

  • vscode-maven: downloaded successfully
  • vscode-java-debug: downloaded successfully
  • vscode-java-dependency-viewer: downloaded successfully
  • vscode-java-redhat: downloaded successfully
  • vscode-coverage-gutters: downloaded successfully
  • vscode-java-test: downloaded successfully

Statictics

Done in 12.88s.
Done in 29.26s.
Done in 26.09s.
Done in 14.95s.

For sure, it is better than 3-8 minutes to download:plugins

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I did the following checks:

  1. the new dependencies are license certified:
Screen Shot 2020-04-27 at 3 08 05 PM
  1. the script correctly works and no longer takes a long time when including vscode-redhat-java:
yarn run v1.22.4
$ theia download:plugins
--- downloading plugins ---
+ vscode-builtin-bat: downloaded successfully
+ vscode-builtin-clojure: downloaded successfully
+ vscode-builtin-css: downloaded successfully
+ vscode-builtin-docker: downloaded successfully
+ vscode-builtin-csharp: downloaded successfully
+ vscode-builtin-coffeescript: downloaded successfully
+ vscode-builtin-handlebars: downloaded successfully
+ vscode-builtin-go: downloaded successfully
+ vscode-builtin-configuration-editing: downloaded successfully
+ vscode-builtin-gulp: downloaded successfully
+ vscode-builtin-debug-auto-launch: downloaded successfully
+ vscode-builtin-cpp: downloaded successfully
+ vscode-builtin-hlsl: downloaded successfully
+ vscode-builtin-grunt: downloaded successfully
+ vscode-builtin-fsharp: downloaded successfully
+ vscode-builtin-json: downloaded successfully
+ vscode-builtin-log: downloaded successfully
+ vscode-builtin-jake: downloaded successfully
+ vscode-builtin-groovy: downloaded successfully
+ vscode-builtin-javascript: downloaded successfully
+ vscode-builtin-ini: downloaded successfully
+ vscode-builtin-html: downloaded successfully
+ vscode-builtin-make: downloaded successfully
x vscode-builtin-node-debug: failed to download with: 404 Not Found
+ vscode-builtin-java: downloaded successfully
+ vscode-builtin-lua: downloaded successfully
+ vscode-builtin-pug: downloaded successfully
+ vscode-builtin-less: downloaded successfully
+ vscode-builtin-theme-tomorrow-night-blue: downloaded successfully
+ vscode-builtin-markdown: downloaded successfully
+ vscode-builtin-perl: downloaded successfully
+ vscode-builtin-powershell: downloaded successfully
+ vscode-builtin-vb: downloaded successfully
+ vscode-builtin-razor: downloaded successfully
+ vscode-builtin-xml: downloaded successfully
+ vscode-builtin-ruby: downloaded successfully
+ vscode-builtin-rust: downloaded successfully
+ vscode-builtin-python: downloaded successfully
+ vscode-builtin-theme-kimbie-dark: downloaded successfully
+ vscode-builtin-shaderlab: downloaded successfully
+ vscode-builtin-r: downloaded successfully
+ vscode-builtin-shellscript: downloaded successfully
+ vscode-builtin-swift: downloaded successfully
+ vscode-builtin-theme-red: downloaded successfully
+ vscode-builtin-sql: downloaded successfully
+ vscode-builtin-scss: downloaded successfully
+ vscode-builtin-objective-c: downloaded successfully
+ vscode-builtin-theme-monokai-dimmed: downloaded successfully
+ vscode-builtin-theme-defaults: downloaded successfully
+ vscode-editorconfig: downloaded successfully
+ vscode-builtin-theme-monokai: downloaded successfully
+ vscode-builtin-theme-abyss: downloaded successfully
+ vscode-builtin-typescript: downloaded successfully
+ vscode-builtin-theme-quietlight: downloaded successfully
+ vscode-builtin-icon-theme-seti: downloaded successfully
+ vscode-builtin-theme-solarized-dark: downloaded successfully
+ vscode-builtin-yaml: downloaded successfully
+ vscode-builtin-merge-conflict: downloaded successfully
+ vscode-eslint: downloaded successfully
+ vscode-builtin-node-debug2: downloaded successfully
+ vscode-builtin-npm: downloaded successfully
+ vscode-builtin-emmet: downloaded successfully
+ vscode-references-view: downloaded successfully
+ vscode-builtin-markdown-language-features: downloaded successfully
+ vscode-maven: downloaded successfully
+ vscode-builtin-typescript-language-features: downloaded successfully
+ vscode-redhat: downloaded successfully
✨  Done in 6.06s.

@vince-fugnitto
Copy link
Member

x vscode-builtin-node-debug: failed to download with: 404 Not Found

@lmcbout there already exists an issue for vscode-builtin-node-debug and it is unrelated to this pull-request: #7488

@lmcbout
Copy link
Contributor

lmcbout commented Apr 27, 2020

@vince-fugnitto

x vscode-builtin-node-debug: failed to download with: 404 Not Found

@lmcbout there already exists an issue for vscode-builtin-node-debug and it is unrelated to this pull-request: #7488

I know there is an issue, but I also asked during the scrum if this issue would be fixed and the answer was : YES. This is why I brought it here.

Copy link
Contributor

@lmcbout lmcbout left a comment

Choose a reason for hiding this comment

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

Code wise look OK
Testing: much better than before
Thanks @marechal-p

@paul-marechal
Copy link
Member Author

paul-marechal commented Apr 27, 2020

@lmcbout it used to crash the download script, thought you were talking about this issue. Will remove that plugin from our examples, since the package hosted on openvsx is bogus. See #7488 (comment)

@vince-fugnitto
Copy link
Member

@lmcbout it used to crash the download script, thought you were talking about this issue. Will remove that plugin from our examples, since the package hosted on openvsx is bogus.

@marechal-p instead of removing it, you can reference the one from GitHub releases which is what the docker images are currently using:

@paul-marechal
Copy link
Member Author

paul-marechal commented Apr 27, 2020

I would have preffered not even caring about this plugin tbh. This is just for the example applications, and is a legacy plugin. Removing it for now.

Unrelated, not touching it.

@paul-marechal
Copy link
Member Author

ci failure unrelated.

@paul-marechal paul-marechal merged commit 498022d into master Apr 27, 2020
@akosyakov akosyakov deleted the mp/download-plugins branch April 28, 2020 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves theia-cli issues related to the theia-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants