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

feat(backend): show error if NodeJS is not installed #779

Merged
merged 7 commits into from
Oct 12, 2023

Conversation

docirl
Copy link
Collaborator

@docirl docirl commented Oct 5, 2023

  • When a user does not have NodeJS installed locally this causes Yeoman UI not to work correctly (blank screen when opening the wizard)
  • When Yeoman UI starts, and before it tries to search for or use a generator, check if node is installed locally by calling getProcessVersions
  • if no node version is returned above, then display a native vscode error message to inform the user

@@ -185,6 +185,7 @@
"dependencies": {
"@sap-devx/webview-rpc": "0.3.1",
"@sap-devx/yeoman-ui-types": "^1.12.2",
"@sap-ux/environment-check": "0.15.64",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not add dependencies on @sap-ux we want the OSS to remain clean of such dependencies

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've removed this and add the code to the npm util.

Copy link
Contributor

@idoprz idoprz left a comment

Choose a reason for hiding this comment

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

Please remove dependencies on @sap-ux

packages/backend/src/panels/YeomanUIPanel.ts Show resolved Hide resolved
packages/backend/test/panels/YeomanUIPanel.spec.ts Outdated Show resolved Hide resolved
packages/backend/test/panels/YeomanUIPanel.spec.ts Outdated Show resolved Hide resolved
packages/backend/test/panels/YeomanUIPanel.spec.ts Outdated Show resolved Hide resolved
packages/backend/test/panels/YeomanUIPanel.spec.ts Outdated Show resolved Hide resolved
packages/backend/src/utils/npm.ts Show resolved Hide resolved

it("should show an error message", () => {
windowMock.expects("showErrorMessage").withExactArgs(messages.nodejs_install_not_found);
void panel.loadWebviewPanel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void panel.loadWebviewPanel();
await panel.loadWebviewPanel();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, thanks

Constants["IS_IN_BAS"] = false;
});

it("should show an error message", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it("should show an error message", () => {
it("should show an error message", async () => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@docirl
Copy link
Collaborator Author

docirl commented Oct 11, 2023

@idoprz I've removed the sap-ux dependency, can you re-review please?

@docirl docirl merged commit cdba893 into master Oct 12, 2023
5 checks passed
@docirl docirl deleted the nodejs-installation-check branch October 12, 2023 14:41
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.

3 participants