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

💡 [Feature]: Add possibility to add custom additional step #217

Open
Adam-it opened this issue Apr 17, 2024 · 12 comments · May be fixed by #408
Open

💡 [Feature]: Add possibility to add custom additional step #217

Adam-it opened this issue Apr 17, 2024 · 12 comments · May be fixed by #408
Assignees
Labels
⭐ enhancement New feature or request 👨‍💻work in progress I am working on it
Milestone

Comments

@Adam-it
Copy link
Member

Adam-it commented Apr 17, 2024

🎯 Aim of the feature

We should add some possibilities, preferably extending it as a setting for the extension, to allow devs to define their own additional setup options.
We could expose a setting in which users could manage a json array that will have a label and command. label will hold the string that will be present in the UI in the scaffolding form and command will be the npm command that should be executed after this option was selected.

example idea for a this kind of json object:

[
  {
    "label": "PnP JS SharePoint",
    "command": "npm install @pnp/sp"
  }
]

We should NOT allow to disable the pre-defined options

📷 Images (if possible) with expected result

No response

🤔 Additional remarks or comments

doing this change we should keep an eye out for this issue #216
if it is already done we should also add a option like visiblity that will allow to list out in as string array components that should have this option visible

@Adam-it Adam-it added ⭐ enhancement New feature or request 🙏 help wanted Open for contributors labels Apr 17, 2024
@Adam-it Adam-it added this to the v3.X milestone Apr 17, 2024
@Adam-it Adam-it modified the milestones: v3.X, v4.X Sep 8, 2024
@ervingayle
Copy link
Contributor

Hi @Adam-it. How did you envision the array being stored?

@Adam-it
Copy link
Member Author

Adam-it commented Jan 16, 2025

Hi @Adam-it. How did you envision the array being stored?

so my idea was to use expose it as and advance setting of an extension.
So the extension may contribute a new setting for it like this
https://code.visualstudio.com/api/references/contribution-points#contributes.configuration

and we add something like

"editPresentation": "multilineText"

the field should be present as a multiline text field. In this setting my idea was to store a JSON array with those settings like

[
  {
    "label": "PnP JS SharePoint",
    "command": "npm install @pnp/sp"
  },
  {
    "label": "PnP React Reusable Controls",
    "command": "npm install ...."
  },
  {  }
]

we should add good docs and description how it works so it is clear how to modify this JSON and add your own nodes to the array with your own custom defined steps which then will be present in the scaffolding form.
What do you think?

@ervingayle
Copy link
Contributor

Hi @Adam-it . Thank you for the example. This helps me. I did not know of this option. I know that you mentioned not allowing for removal of the default ones but this json array would only specify the additional packages that someone would want installed. Yes, I believe that this addition would require good docs with examples to effectively help everyone.

@Adam-it
Copy link
Member Author

Adam-it commented Jan 16, 2025

Hi @Adam-it . Thank you for the example. This helps me. I did not know of this option. I know that you mentioned not allowing for removal of the default ones but this json array would only specify the additional packages that someone would want installed. Yes, I believe that this addition would require good docs with examples to effectively help everyone.

Exactly,
We could even do a short demo about this feature on the PnP community call once it's ready.
So @ervingayle do you want to get assigned and try to implement this one?

@ervingayle
Copy link
Contributor

Yes, please assign this to me.

@Adam-it Adam-it added 👨‍💻work in progress I am working on it and removed 🙏 help wanted Open for contributors labels Jan 16, 2025
@Adam-it
Copy link
Member Author

Adam-it commented Jan 16, 2025

Yes, please assign this to me.

All yours 👍

@ervingayle
Copy link
Contributor

Hi @Adam-it. I have made progress with this item but I do have some issues with the implementation. Currently I tried with selecting React and React Dom, PnP and then I added a new "Additional Step" called Install custom steps. This reads the setting value and then I loop through the json. This works. However, what I am seeing is that the React and React Dom step is being bundled with the install dependencies initial powershell task where npm i is executed. Every additional custom step is created in a new terminal window and installed simultaneously. In some cases the package is installed and no errors are shown, folder in node_modules but the package entry is not in package.json. I have tested this about 15 times so far. Do you know what would cause this?

Also, I intended to show what would be installed in the scaffolding form but passing the setting value to the view didn't work as I expected so for now I have reverted that code.
I have not opened a pull request as yet. Code here

@Adam-it
Copy link
Member Author

Adam-it commented Jan 29, 2025

@ervingayle thanks for reaching out.
So I am not 100% if I understand everything you wrote but I think I kinda get what the problem is. From what I understand the code you added that should loop over the custom additional steps simply fires in parralel rather than sequentially and that is why you see multiple terminal opening with multiple npm i... commands running.

I checked your commits and most probably I would investigate this part
https://github.com/ervingayle/vscode-viva/blob/74c58c5de15c4a6026739de35cd7b69fae2912e8/src/extension.ts#L96-L100

Here you have a forEach that esentially returns multiple async functions and each of those run in parallel. It is not a loop where every iteration of the loop awaits to finish until the next iteration is taken. I would suggest to refactor it to a for loop and reacheck.

Let me know if it helps.

@ervingayle
Copy link
Contributor

Hi @Adam-it . Thank you for the response. I tried with a for loop and get similar behavior. I realize the behavior that I am seeing is due to the fact that I set a unique title for each package that is set in the extension setting and the TerminalCommandExecutor is looking at title/name value to determine if a new terminal should be created. I don't change the name for the React, React Dom install option but I did for the custom steps which is why I was confused at first. Its tested and working now.

@ervingayle
Copy link
Contributor

Hi @Adam-it . Thanks for your help. PR submitted for this item.

@Adam-it
Copy link
Member Author

Adam-it commented Jan 30, 2025

Hi @Adam-it . Thanks for your help. PR submitted for this item.

You Rock 🤩

@ervingayle
Copy link
Contributor

Hi @Adam-it . Thank you for all your help. The guidance you provided helped. I have created a new PR for this issue.

@Adam-it Adam-it linked a pull request Feb 15, 2025 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐ enhancement New feature or request 👨‍💻work in progress I am working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants