search

sapio-lang / bitcoin-core-ts

A port of bitcoin-core that will (over time) become TS friendly.
6 stars 2 forks source link

Add package-lock.json or yarn.lock file #3

Closed wbobeirne closed 2 years ago

wbobeirne commented 2 years ago

Dependencies should be locked to prevent non-deterministic builds and vulnerabilities or attacks from new versions of dependencies before they're properly vettted.

Personally I'm a fan of yarn but it doesn't really matter either which way, as long as all the commands indicate you use one or the other for development / CI.

JeremyRubin commented 2 years ago

it's not clear to me how package.lock works for non-binary packages... should it be comitted or open for merging with other versions of deps?

JeremyRubin commented 2 years ago

i am using npm for this since it's what was used before, happy to switch to yarn if that's the better choice.

wbobeirne commented 2 years ago

NPM and package-lock.json is totally fine, no need for yarn. It's easier for most contributors since it doesn't require a separate binary to install, so that's all good, though I've found performance to be better with yarn.

As for committing it, it's intended to be committed to source, and does not affect things for consumers of the module, only developers. Consumers will still reconcile the packages according to package.json's dependency semvars, but this makes it so that when you npm install in the project, you get the same modules as everyone else.

This is really helpful for reproducible builds (e.g. TypeScript version is exactly the same, so our dist folders will output the same transpiled code) and preventing supply chain attacks (Without package-lock.json, anyone who runs npm install will get the latest version of a module without checking an integrity hash, which might be malicious.)

JeremyRubin commented 2 years ago

added this to #4

JeremyRubin commented 2 years ago

let's also set a goal to make this library as dependency free as possible. it isn't using much external other than semver, and i think the others (like logging?) could be made optional / dependency injected by the user.

wbobeirne commented 2 years ago

Love it, I'm game to help yank some.

JeremyRubin commented 2 years ago

@wbobeirne the biggest ones i think that make sense to get rid of are the uphold and debugnyan ones. Neither are particularly maintained, nor widely used. This only gets rid of logging, which we can solve via dependency injection and default to something with console.log / no logging. Would you like to do a PR with that?

let's upgrade json-bigint to the latest version (some DoS vulns it looks like, but we only parse stuff coming from a core node). we can leave lodash for now, but it doesn't look too heavily used to replace (+ anyone depending on this probably already has lodash...)

wbobeirne commented 2 years ago

Sounds good to me, I'll move the discussion to a new issue.