Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

We check our node_modules folder into source control

jackfranklin.co.uk

Post reply on HN
Print view View original

101–110 of 241 posts

Re: We check our node_modules folder into source control

#101

Post by johnnypangs »

scrollaway wrote: post #100

This is all horrible advice and other commenters have rightfully pointed this out already so I won't repeat it more… (okay, once more: This is all horrible advice, don't do that) But, there is one thing I like from this, which is git diffs showing the actual final code diff when you upgrade dependencies. Of course, this being horrible advice, it ignores how many JS packages ship minified which would make the diff as…

Most node modules (and I’m pretty sure is best practice) do not minify published node module code. The user should decide if they want to/how to minify. I often go in an read node module code and although it might have been transpiled for compatibility, it is not minified.

Re: We check our node_modules folder into source control

#102

Post by nickkell »

I know pulling in lots of dependencies from various anonymous authors is a security risk. Can you be sure that all of the code has been vetted? This seems exacerbate this. You’re allowing developers to check in anything without oversight and it will be ignored just because it’s in this particular folder

Re: We check our node_modules folder into source control

#103

Post by danShumway »

d3ad1ysp0rk wrote: post #49
danlugo92 wrote: post #7

No, this is what the lock file is for.

Note the other child comments of this; NPM has failed to make the lockfile reliable across systems and versions. Here's an example from March that isn't fixed: https://github.com/npm/cli/issues/2846 The fact that you have so many people believing "nuke node_modules and delete package-lock.json" is a reasonable step in diagnosing an error is damning to NPM. We don't check in our node_modules, but "use the lockfile" is…

I honestly don't understand how people get this impression of lockfiles as being perfectly reliable. How are they not occasionally bitten by these bugs? Maybe I'm just unlucky, but I'm a little jealous of these developers who apparently are good enough managing/updating their dependencies and keeping their count low enough that they've just never run into problems like this before.

Lockfile v1 literally ignores pinned versions of dependencies if the package.json specifies a fuzzy version number[0], and the advice of the npm team was, "it's fine, everyone will just bump a major version number of npm." And to this day, I still don't know what the expected behavior is, there really isn't a list anywhere about when the lockfile is and isn't supposed to be respected. So it's not really surprising to me that people distrust version pinning, and I always feel like I'm kind of living in a different world when people say that lockfiles just solve everything.

[0]: https://github.com/npm/cli/issues/564#issuecomment-921314014

Re: We check our node_modules folder into source control

#105

Post by 20after4 »

awill88 wrote: post #72

Can’t you like, cache the node_modules folder on CI builds? I dunno, seems gross, unless you’re on a project with minimal deps or very meticulous about which deps you leverage. I am just one of those people who are constantly trying to upgrade dependencies anyways (cautiously of course) as to avoid vulnerabilities. That said, I see the point, it’s interesting..

Upgrading could introduce vulnerabilities just as easily as fixing them.

Re: We check our node_modules folder into source control

#106

Post by Gigachad »

nickkell wrote: post #102

I know pulling in lots of dependencies from various anonymous authors is a security risk. Can you be sure that all of the code has been vetted? This seems exacerbate this. You’re allowing developers to check in anything without oversight and it will be ignored just because it’s in this particular folder

That’s already exactly the same risk almost all web developers take currently. Yes it is a real threat, but it’s too hard to deal with and not often exploited.

Re: We check our node_modules folder into source control

#107

Post by StreamBright »

mohanmcgeek wrote: post #55

The listed reasons are insufficient and we could achieve many of these by just pinning our dependencies versions. If we did this, our git repo after a few commits will tend towards a gazillion GBs. Costs outweigh the benefits, if at all there are any. Horrible advice. Don't break the industry practice and check-in your node_modules

Lets address some points instead of cargo culting.

How would you go about the left pad issue?

Re: We check our node_modules folder into source control

#108

Post by xboxnolifes »

lf-non wrote: post #96
emteycz wrote: post #91

Earlier quoted context omitted.

You can have multiple commits in a PR

Yes, but a lot of people prefer to have every commit in master in a buildable state. Otherwise things like git bisect become a lot more tedious.

> have every commit in master

Yes, but a PR commit is not in master.

Re: We check our node_modules folder into source control

#109

Post by StopHammoTime »

I don't agree that it should be checked into source control. I do believe it should be cached somewhere. How much bandwidth of popular sites are used by redundant actions. A single request for a 2GB archive is much better than a 1,000,000 small requests that are all a few KBs or MBs in size.

Re: We check our node_modules folder into source control

#110

Post by wruza »

ilumanty wrote: post #13

Yarn offers "Plug'n'play" mode since v2, which basically promotes what the author says. It takes the idea further: dependencies are stored as zip archives instead of thousands of small files, which reduces the "git noise" and actually makes this viable as a performant workflow. https://yarnpkg.com/features/pnp

Yarn pnp was in a poor state tooling-wise last time I checked it a year or so ago. Too many tools depend on ./node_modules/ (or actually an entire module.paths thing) to exist on a filesystem. Was it resolved somehow?
Post reply on HN