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

🐞 Bug report: Terminal receives "pm i" instead of "npm i" when choosing npm installation option #246

Closed
Saurabh7019 opened this issue Jun 5, 2024 · 5 comments
Assignees
Labels
🐞 bug Something isn't working πŸ‘¨β€πŸ’»work in progress I am working on it
Milestone

Comments

@Saurabh7019
Copy link
Contributor

⭐ Priority

(Low)☹️ Something is a little off

πŸ“ Describe the bug

Creating a new project and choosing npm i after the project is created doesn't work as expected. The terminal receives pm i instead of npm i.

πŸ‘£ Steps To Reproduce

  1. Create a new project.
  2. Provide the necessary project details.
  3. Select the option to 'Run npm i after the project is created'.
  4. Observe the terminal output to see if 'npm i' command is executed correctly.

πŸ“œ Expected behavior

The terminal should receive npm i when the project is created.

πŸ“· Screenshots

image

❓SharePoint Framework Toolkit version

3.2.1

❓Node.js version

v18.17.1

πŸ€” Additional context

No response

@Saurabh7019 Saurabh7019 added the 🐞 bug Something isn't working label Jun 5, 2024
@nicodecleyre
Copy link
Contributor

Hi @Saurabh7019,

Been testing this one with the latest version 3.2.1 and with node.js version 18.17.1, but getting npm i when a new project is created.
image

Can you double check if there is terminal.sendText('npm i'); in src/extension.ts?

@Adam-it
Copy link
Contributor

Adam-it commented Jun 10, 2024

@nicodecleyre thank you jumping in on this. You Rock 🀩
Basically this error 'might not always happen' πŸ˜‰. I know seems strange but I will explain why.
How I developed it is that is not waiting for terminal to show
image

So I create a new terminal instance and send a text to it without waiting for it to 'boot up'. VS Code terminal is integrated so if someone has the terminal customized (for example uses POSH in PS terminal) it might take a bit some time for new terminal to show. From what I am aware the sendText method works as a stream so some part might be send but not 'recieved' by the terminal in time as it was not up yet.
This is my bad and I did this implementation totally wrong assuming everything will go without failures.

What we should do is do something like it is done in runCommand method in TerminalCommandExecuter.ts

Here not only I create the terminal but wait for it to show before I pass the command.

Ideally what I was thinking of this fix was to reuse the TerminalCommandExecuter in the extension.ts to unify the way we pass commands to terminal in all places and make it more bulletproof.

@Saurabh7019
Copy link
Contributor Author

Hi @Saurabh7019,

Been testing this one with the latest version 3.2.1 and with node.js version 18.17.1, but getting npm i when a new project is created. image

Can you double check if there is terminal.sendText('npm i'); in src/extension.ts?

I have customized my PowerShell prompt using PSReadLine and included the CLI.Microsoft365.PowerShell.Predictor module in my startup profile. This customization is causing a significant delay in loading my personal and system profiles, which could be why I am consistently reproducing the issue.

You might not experience this issue even with the same customization because of your lightning-fast machine. I remember your CLI tests execution times were impressively quick. :)

@Adam-it Adam-it added this to the v3.X milestone Jun 10, 2024
@Adam-it
Copy link
Contributor

Adam-it commented Jun 10, 2024

You might not experience this issue even with the same customization because of your lightning-fast machine. I remember your CLI tests execution times were impressively quick. :)

Running CLI tests in light speed be like
StarWarsLightGIF

@Adam-it Adam-it self-assigned this Jul 26, 2024
Adam-it added a commit that referenced this issue Aug 5, 2024
…#246 (#280)

## 🎯 Aim

The aim is to refactor the way start and run commands in integrated VS
Code terminal. The idea is to aling the approach by using the dedicated
Terminal service which was developed for exactly that.

## βœ… What was done

- [X] Removes all direct usage of `terminal` and refactors it to same
approach

## πŸ”— Related issue

Closes: #246
@Adam-it
Copy link
Contributor

Adam-it commented Aug 5, 2024

PR merged with the fix.
Soon I will release a pre-release with the fix

@Adam-it Adam-it closed this as completed Aug 5, 2024
Adam-it added a commit that referenced this issue Aug 6, 2024
…#246 (#280)

## 🎯 Aim

The aim is to refactor the way start and run commands in integrated VS
Code terminal. The idea is to aling the approach by using the dedicated
Terminal service which was developed for exactly that.

## βœ… What was done

- [X] Removes all direct usage of `terminal` and refactors it to same
approach

## πŸ”— Related issue

Closes: #246
Adam-it added a commit that referenced this issue Sep 8, 2024
…#246 (#280)

## 🎯 Aim

The aim is to refactor the way start and run commands in integrated VS
Code terminal. The idea is to aling the approach by using the dedicated
Terminal service which was developed for exactly that.

## βœ… What was done

- [X] Removes all direct usage of `terminal` and refactors it to same
approach

## πŸ”— Related issue

Closes: #246
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working πŸ‘¨β€πŸ’»work in progress I am working on it
Projects
None yet
Development

No branches or pull requests

3 participants