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

repo: update bundled builtins #10049

Merged
merged 1 commit into from
Apr 12, 2022
Merged

repo: update bundled builtins #10049

merged 1 commit into from
Apr 12, 2022

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Sep 3, 2021

What it does

Fixes: #9985.

The pull-request updates the list of builtins (bundled plugins) which are used in the repository for test purposes (ultimately the bundled plugins used by both the browser and electron examples). It is important to update the builtins following a default api bump (#9959) so we can identify issues more easily and work towards increased api compatibility.

The update includes the use of the builtins-extension-pack (which can be resolved at builtime to the latest supported version based on the api), and the use of an exclude list (to exclude the list of plugins we do not want as part of the application).

Known Issues:

How to test

  • perform yarn download:plugins - the pack should be installed and valid plugins should be resolved.
  • start the application.
  • identify any potential issues or incompatibilities.

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the builtins Issues related to VS Code builtin extensions label Sep 3, 2021
@vince-fugnitto vince-fugnitto self-assigned this Sep 3, 2021
@msujew msujew self-requested a review September 3, 2021 17:09
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The npm extension does not seem to be compatible anymore:

root ERROR Failed to fetch children for 'npm' TypeError: Cannot read property 'onCancellationRequested' of undefined
    at v.getTsConfigsInWorkspace (/workspace/theia/plugins/vscode.typescript-language-features/extension/dist/extension.js:45:69642)
    at v.getAllTsConfigs (/workspace/theia/plugins/vscode.typescript-language-features/extension/dist/extension.js:45:68703)
    at v.provideTasks (/workspace/theia/plugins/vscode.typescript-language-features/extension/dist/extension.js:45:68068)
    at TaskProviderAdapter.provideTasks (/workspace/theia/packages/plugin-ext/lib/plugin/tasks/task-provider.js:25:46)
    at TasksExtImpl.$provideTasks (/workspace/theia/packages/plugin-ext/lib/plugin/tasks/tasks.js:138:28)
    at RPCProtocolImpl.doInvokeHandler (/workspace/theia/packages/plugin-ext/lib/common/rpc-protocol.js:232:23)
    at RPCProtocolImpl.invokeHandler (/workspace/theia/packages/plugin-ext/lib/common/rpc-protocol.js:217:41)
    at RPCProtocolImpl.receiveRequest (/workspace/theia/packages/plugin-ext/lib/common/rpc-protocol.js:181:33)
    at RPCProtocolImpl.receiveOneMessage (/workspace/theia/packages/plugin-ext/lib/common/rpc-protocol.js:145:26)
    at /workspace/theia/packages/plugin-ext/lib/common/rpc-protocol.js:69:48

The npm view does not get populated anymore, although other features, like debugging build scripts still work correctly.

@vince-fugnitto
Copy link
Member Author

The npm extension does not seem to be compatible anymore:

Right, it’s the known issue I noticed during testing and added initially in the pull-request description.

@marcdumais-work
Copy link
Contributor

We have the option to pin misbehaving built-in extensions to slightly older versions, while we work to resolve the issues.

@vince-fugnitto vince-fugnitto force-pushed the vf/vscode-builtins branch 2 times, most recently from 9b59c15 to 6b8257b Compare September 8, 2021 16:25
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me. As far as I could tell, every built-in extension seems to work as expected, now that the npm issue has been resolved. 👍

@JonasHelming
Copy link
Contributor

@vince-fugnitto Can this be merged?

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto Can this be merged?

No, the api-tests are failing with the newer builtins (ex: typescript) so additional time will need to be spent to fix them.

The commit updates the list of `builtin` plugins included with the app
(bundled). The changes make use of the extension-pack (which will get
resolved to the latest api version), and the list of excluded plugin ids
which should not be included.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have particularly tested the new version of the references-view and found that it continues to work for references, implementations, and call hierarchy as expected.

@vince-fugnitto vince-fugnitto merged commit 2a8f93e into master Apr 12, 2022
@vince-fugnitto vince-fugnitto deleted the vf/vscode-builtins branch April 12, 2022 15:34
@github-actions github-actions bot added this to the 1.25.0 milestone Apr 12, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update VS Code built-in extensions
5 participants