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

upgrade NPM builtin version to 1.44.2 #8000

Merged
merged 2 commits into from
Jun 16, 2020
Merged

upgrade NPM builtin version to 1.44.2 #8000

merged 2 commits into from
Jun 16, 2020

Conversation

elaihau
Copy link
Contributor

@elaihau elaihau commented Jun 11, 2020

upgrade NPM builtin version to 1.44.2

added detail to the task config schema

  • In VS Code a detail property can be added to the contributed and configured tasks. The content of that detail property is displayed as part of the task quick open menu items. With changes in this commit, detail is added to the task config schema.

Signed-off-by: Liang Huang [email protected]

How to test

  • Start a workspace where scripts are defined in package.json.
  • Check if all scripts are available in the NPM VIEW, and if the scripts can be started from the view.

Review checklist

@elaihau elaihau requested review from lmcbout and evidolob June 11, 2020 18:08
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.

Thank you for the contribution @elaihau!
I verified the changes and it works correctly:

  • no Task2 npm error logged
  • the npm scripts tree-view is correctly populated (using theia as workspace)
  • executing a script from the npm scipts tree-view works correctly

@vince-fugnitto vince-fugnitto added builtins Issues related to VS Code builtin extensions tasks issues related to the task system and removed tasks issues related to the task system labels Jun 11, 2020
@akosyakov
Copy link
Member

akosyakov commented Jun 12, 2020

@elaihau Where do we use Task2.detail in the task extension? We should avoid just stubbing APIs when it is possible.

@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jun 12, 2020
@elaihau
Copy link
Contributor Author

elaihau commented Jun 12, 2020

Where do we use Task2.detail in the task extension? We should avoid just stubbing APIs when it is possible.

@akosyakov
Task2.detail is not used in Theia's task extension at all.
However it is used as a property of the Task class in some VS Code extensions that are recently updated :)

In this case, having Task2.detail defined or not-defined does not matter to us because

  • Theia's task extension does not use the detail
  • Task2.detail is optional anyway, so extensions can run without throwing errors even if Task2.detail is not defined. (I will do more testing to be 100% sure)

In my opinion, we have to add Task2 to the Theia plugin's API to make extensions such as npm work. Having Task2.detail defined makes Theia API becomes more compatible with VS Code. So... why not ? :)

What do you think ?

@elaihau
Copy link
Contributor Author

elaihau commented Jun 12, 2020

OK the built-in npm breaks more than I thought.

As the GIF shows the "Run" and "Refresh" icons are invisible after upgraded to 1.44.2
Peek 2020-06-12 11-08

I will see what I can do to bring them back

@vince-fugnitto
Copy link
Member

OK the built-in npm breaks more than I thought.
As the GIF shows the "Run" and "Refresh" icons are invisible after upgraded to 1.44.2
I will see what I can do to bring them back

@elaihau I believe the issue is that we do not yet support codicons as defined in the package.json of extensions, something I noticed when I created my own extension. It looks like the npm extension uses these codicons now:

microsoft/vscode@573fe9e#diff-45afd42bd496e3cf085556a5c1d1a632R57-R60

@elaihau
Copy link
Contributor Author

elaihau commented Jun 12, 2020

OK the built-in npm breaks more than I thought.
As the GIF shows the "Run" and "Refresh" icons are invisible after upgraded to 1.44.2
I will see what I can do to bring them back

@elaihau I believe the issue is that we do not yet support codicons as defined in the package.json of extensions, something I noticed when I created my own extension. It looks like the npm extension uses these codicons now:

microsoft/vscode@573fe9e#diff-45afd42bd496e3cf085556a5c1d1a632R57-R60

Thank you for information @vince-fugnitto !
If the codicon support won't be added in this release I guess I should close this pull request for now? We don't want to upgrade npm to a version that only works partially in Theia. What do you think?

@vince-fugnitto
Copy link
Member

Thank you for information @vince-fugnitto !
If the codicon support won't be added in this release I guess I should close this pull request for now? We don't want to upgrade npm to a version that only works partially in Theia. What do you think?

@elaihau I don't think there's an issue upgrading the builtin in this repo, downstream projects can choose their own compatible versions. At least with the new npm we can find other potential issues faster, missing icons can be tracked but aren't a showstopper (it is potentially an issue with other builtins as well).

@akosyakov
Copy link
Member

Task2.detail is not used in Theia's task extension at all.

How hard to add it? If it is hard there should be an issue at least that we are of the issue. Stubbing VS Code API does not mean that they are implemented and does not make Theia compatible, since whenever such APIs are used they don't make any effect.

elaihau added 2 commits June 13, 2020 18:01
- The vs code built in extension `npm` does not work with Theia's plugin
extension due to not being able to find the constructor of `Task2`. This
pull request addes the missing constructor, and upgrades the version of
`npm` in Theia.

Signed-off-by: Liang Huang <[email protected]>
- In VS Code a `detail` property can be added to the contributed and configured tasks. The content of that `detail` property is displayed as part of the task quick open menu items. With changes in this commit, `detail` is added to the task config schema.

Signed-off-by: Liang Huang <[email protected]>
@elaihau
Copy link
Contributor Author

elaihau commented Jun 14, 2020

Task2.detail is not used in Theia's task extension at all.

How hard to add it? If it is hard there should be an issue at least that we are of the issue. Stubbing VS Code API does not mean that they are implemented and does not make Theia compatible, since whenever such APIs are used they don't make any effect.

@akosyakov I added one more commit to this pull request to support using detail in Theia's task extension.

GIF is from Theia with this change.
Peek 2020-06-13 21-57

@akosyakov
Copy link
Member

@vince-fugnitto Could you retest it please? Please also while reviewing of VS Code compatibility make sure that such APIs are used. 🙏

@akosyakov akosyakov requested a review from vince-fugnitto June 14, 2020 09:32
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.

The changes work well for me with the latest changes 👍
I verified that:

  • npm scripts is correctly populated
  • executing a script works correctly
  • there are no longer errors
  • run task continues to work as expected

@elaihau elaihau merged commit 628f724 into master Jun 16, 2020
@elaihau elaihau deleted the Liang/npmV44 branch June 16, 2020 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins Issues related to VS Code builtin extensions vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPM scripts not available with builtin version 1.44.2
3 participants