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 setting to enable/disable the .nvmrc file creation #285

Closed
GuidoZam opened this issue Jul 29, 2024 · 14 comments · Fixed by #316
Closed

💡 [Feature]: Add setting to enable/disable the .nvmrc file creation #285

GuidoZam opened this issue Jul 29, 2024 · 14 comments · Fixed by #316
Assignees
Labels
Milestone

Comments

@GuidoZam
Copy link
Contributor

GuidoZam commented Jul 29, 2024

🎯 Aim of the feature

Add a new additional step setting when scaffolding a new project, the additional YES/NO switch will have a label 'Create node version manager configuration file (.nvmrc)'. This setting will be added after the already existing ones:

image

The setting, if set to YES, will create the .nvmrc file when scaffolding the new project.

Other than adding the new additional step two new extension settings will be added:

  • createNodeVersionFileDefaultValue: this setting will define what's the default value for the additional scaffold step. By default the value will be NO.
  • nodeVersionManagerFile: this setting will define what Node version configuration file to use. If the user has selected NVM as Node versioning software this setting won't display, instead if the NVS versioning software is used the user can choose the configuration file to use, the available choices are:
    • .nvmrc
    • .node-version

📷 Images (if possible) with expected result

No additional remarks

🤔 Additional remarks or comments

No additional remarks

@Adam-it
Copy link
Contributor

Adam-it commented Jul 29, 2024

@GuidoZam thanks for this suggestion 👍. I think it is a really awesome idea, let's give it though and discuss different options and then prototype(draft/spec) how it should be done before we open it up.
I will try to get back with more comments and questions regarding this issue later during the day 👍

@Adam-it
Copy link
Contributor

Adam-it commented Jul 29, 2024

@GuidoZam some feedback from my side on this idea:

  1. This new setting seems strictly related to project scaffolding. VS Code extension setting are usually something most of us (developers) do not change 😉 so if we put this there it will get little visibility. Besides that we already have some additional 'settings' that one may be interested when scaffolding new project and this may be found in the 3rd step of the scaffolding form. IMO I would rather add this as a new option in the 'Additional Steps' section in the scaffolding form. Just another YES/NO switch with label like 'create node version manager configuration file (.nvmrc)' or something similar.
    image
  2. Then if the above suggestion would be fine with you in the VS code extension settings I would rather add a setting if this should be 'turned on' (so set as YES) by default. Otherwise by default it will be NO which should be this default... default value 😅
  3. SPFx Toolkit supports either NVM or NVS. NVM only supports .nvmrc (at least this is what I think 😅), but NVS supports both .nvmrc and .node-version. Maybe we should also include additional setting, enabled only when someone selected NVS as preferable node version manager, to select if .nvmrc should be created (default) or .node-version.
    image

What do you think of the above 👆? IMO applying those suggestions to your idea will make this more flexible and visible.

As for the additional remark. For windows one may use nvm-windows which is also the same as nvm
https://github.com/coreybutler/nvm-windows?tab=readme-ov-file
in this case nvm use respect the .nvmrc file. At least I think it does 😅. It does seem to work 👍

@Adam-it Adam-it added this to the v3.X milestone Jul 29, 2024
@GuidoZam
Copy link
Contributor Author

@Adam-it my reply to your points:

  1. You're right, adding a new toggle in the 'Additional Steps' section definitely seems a better way and place to manage this.
  2. Seems a great idea, and I think it's even more useful and personalizable this way.
  3. Also this point seems legit. The .nvmrc file will be the default for both nvm and nvs while when the user select nvs as default Node version manager it will also make available an additional setting where it can select between .nvmrc or .node-version.

@Adam-it
Copy link
Contributor

Adam-it commented Jul 29, 2024

Awesome 🤩😍.
@GuidoZam may I kindly ask you to update the initial post of this issue with what we clarified 🙏. You may of course copy paste most of my points and this will be our spec (prototype) of this feature.
After that let's open this up.
Would you like to get assigned?

@GuidoZam
Copy link
Contributor Author

@Adam-it I've updated the issue, I think it's ok this way but if you need more clarification just tell me 😄
For the assignment: yes, assign this to me, I will do my best to improve this awesome extension!

@Adam-it
Copy link
Contributor

Adam-it commented Jul 29, 2024

@Adam-it I've updated the issue, I think it's ok this way but if you need more clarification just tell me 😄 For the assignment: yes, assign this to me, I will do my best to improve this awesome extension!

AWESOME!. All yours 👏😍🤩

@Adam-it Adam-it modified the milestones: v3.X, v4.X Sep 8, 2024
@Adam-it
Copy link
Contributor

Adam-it commented Sep 23, 2024

Hi @GuidoZam
Hows the work coming up with this issue?
Do you need any additional help from my side?
This year the SPFx Toolkit repo will participate in hacktoberfest and this issue will count so if you are also planning to join this event please do remember to rise a PR during October (no sooner no later 😉)

@GuidoZam
Copy link
Contributor Author

Hey @Adam-it!
The work is almost done, I'm having some difficulties with the dynamic update of the setting that specify which Node version file to use.
Do you have any advice on that?

Good to know about the hacktoberfest, I will surely commit all the work in time to participate!

@Adam-it
Copy link
Contributor

Adam-it commented Sep 24, 2024

I'm having some difficulties with the dynamic update of the setting that specify which Node version file to use. Do you have any advice on that?

What do you mean by dynamic update?

@GuidoZam
Copy link
Contributor Author

I mean to hide or show the nodeVersionConfigurationFile, as mentioned in the issue description:

nodeVersionConfigurationFile: this setting will define what Node version configuration file to use. If the user has selected NVM as Node versioning software this setting won't display, instead if the NVS versioning software is used the user can choose the configuration file to use.

Once done that I will commit the changes!

@Adam-it
Copy link
Contributor

Adam-it commented Sep 25, 2024

maybe it doesn't need to be dynamically show/hidden only when NVS. IMO this setting may always be present but in its description we may just include information that it applies only when NVS manager is selected and for NVM it won't be considered

@GuidoZam
Copy link
Contributor Author

That was my plan B! 😄
I'll try to reserve some time to finalize this next week, I'll keep you updated!

This was referenced Oct 5, 2024
@GuidoZam
Copy link
Contributor Author

GuidoZam commented Oct 5, 2024

Hey @Adam-it I finally commited the changes!
Everything is working fine.

JFYI I had to cancel the previous PR and recreate a new one because there was some issue with some previous commit.

@Adam-it Adam-it linked a pull request Oct 13, 2024 that will close this issue
3 tasks
Adam-it pushed a commit that referenced this issue Oct 14, 2024
## 🎯 Aim

Added a new additional step setting when scaffolding a new project to
specify if the node version manager configuration file should be created
or not.

## 📷 Result

![image](https://github.com/user-attachments/assets/e99d1ee8-2c0a-4070-8252-40086a50b222)

![image](https://github.com/user-attachments/assets/bca41a6a-7247-49e0-a51c-adb77d86982b)

## ✅ What was done

- [X] Added a new project additional step to create or not the node
version manager configuration file.
- [X] Added new VSCode setting `createNodeVersionFileDefaultValue` to
define the default value of the new project additional step.
- [X] Added new VSCode setting `nodeVersionManagerFile` to specify which
configuration file to be created.

## 🔗 Related issue

Closes: #285
@Adam-it
Copy link
Contributor

Adam-it commented Oct 14, 2024

Awesome work 👏👏👏. PR merged and I will try to make a new pre-release with this ASAP 👍

@Adam-it Adam-it closed this as completed Oct 14, 2024
Adam-it pushed a commit that referenced this issue Oct 19, 2024
## 🎯 Aim

Added a new additional step setting when scaffolding a new project to
specify if the node version manager configuration file should be created
or not.

## 📷 Result

![image](https://github.com/user-attachments/assets/e99d1ee8-2c0a-4070-8252-40086a50b222)

![image](https://github.com/user-attachments/assets/bca41a6a-7247-49e0-a51c-adb77d86982b)

## ✅ What was done

- [X] Added a new project additional step to create or not the node
version manager configuration file.
- [X] Added new VSCode setting `createNodeVersionFileDefaultValue` to
define the default value of the new project additional step.
- [X] Added new VSCode setting `nodeVersionManagerFile` to specify which
configuration file to be created.

## 🔗 Related issue

Closes: #285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants