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

fix: don't have node as a dep and add a lock file #77

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tefkah
Copy link
Contributor

@tefkah tefkah commented Apr 14, 2022

I could not develop this version on my local PC because it attempted to install node as a dependency, which I have never seen anyone do but apparently it's not that common.

I would advise against this and just use an .nvmrc or something, for one because this fails on some systems (M1 macs) and secondly because mixing even more versions of node does not seem like a good idea to me.

But it's up to you!

I would recommend adding a lock file though. I like yarn, hence that one, but you should commit it to the repo.

Lovely plugin by the way!

@stefanopagliari
Copy link
Owner

@ThomasFKJorna thank you for this. I'm very much a beginner when it comes to js/typescript so I'm not exactly sure what is the issue you are describing and the solution. Could you please help me to understand what adding the lock file and removing node from package.json does?

@tefkah
Copy link
Contributor Author

tefkah commented May 5, 2022

Sure! Sorry for the late response!

The reason why you would add a lock file is to assure everyone who installs the project using yarn or npm install has the same dependencies installed. If you just specify the dependencies in the package.json, you can run into the problem of two packages requiring the same dependency, but need a different version. When this happens, because your package manager downloads a lot of these at the same time (and other factors), there is no guarantee that all the dependencies will try to use the correct version they need, leading to very frustrating installation procedures that are extremely difficult to debug.

Now, this is not that big of a problem here, as there's only a couple of dependencies, but it's good practice.

As for the node version: since everyone who tries to npm install something already has node on their computer, there is little need to include it. Of course, you want to synchronize versions, but this is better done through a tool such as nvm than through manually downloading another binary, as this can add a lot of size when you have a lot of projects on your pc. It's not that bad, but I've never seen anyone else do it.

@stefanopagliari
Copy link
Owner

@ThomasFKJorna, sorry I did not have a chance to get back to this until now.
Thanks for explaining this to me. You make a lot of sense but I have to admit that I'm not really sure what is required to implement the two steps.

If I understand correctly this pull request, you are suggesting I remove "node": "^17.2.0", from package.json?
Since your message, I have added a couple of dependencies (a colour picker and an html-to-markdown formatter).

Regarding the installation of nvm, is it enough to download and install it in the folder of the package or am I required to do something else?

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.

2 participants