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

Missing yarn.lock / type errors #32350

Closed
joshribakoff opened this issue Jul 11, 2019 · 11 comments
Closed

Missing yarn.lock / type errors #32350

joshribakoff opened this issue Jul 11, 2019 · 11 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@joshribakoff
Copy link

joshribakoff commented Jul 11, 2019

You all apparently do not like lock files: 9d263b4#diff-a3e03be55c12fe638fe1e1c5aa684b9a

When cloning this repo, it installs the latest version of every dev dependency & the project has type errors as a result.

In VScode, I selected "use workspace version" which is just latest (3.6.0-dev), seeing as you have no lock file. The code outright has type errors without modifying anything:

Screen Shot 2019-07-11 at 9 05 36 AM

To me it seems unreasonable to not have a lock file. Can you please revert this & add a proper lock file, so potential contributors can build the project correctly? Alternatively, can someone please explain why you don't lock the dependencies & what I'm doing "wrong"? I read your readme & contributor guide, both of which seem to be totally lacking any instructions about these matters.

@sandersn
Copy link
Member

Can you try these steps and see if they work?

$ git clone git@github.com:microsoft/TypeScript.git
$ cd TypeScript
$ npm install
$ gulp
$ gulp runtests-parallel

@sandersn sandersn added the Question An issue which isn't directly actionable in code label Jul 11, 2019
@orta
Copy link
Contributor

orta commented Jul 12, 2019

We removed the lock file being generated because if you had done a yarn install, then came back after a while and git pull'd the yarn.lock isn't updated so you're now locked to a different version of node modules to everyone else.

@joshribakoff
Copy link
Author

joshribakoff commented Jul 13, 2019

Those are the steps I followed, I tried with yarn and with npm, in both cases I get the latest unstable dependencies which results in type errors in the editor.

The explanation for not committing the lock file unfortunately do not make any sense to me, respectfully. The reason lock files exist are to keep everyone locked on the same version, as the name implies. By not committing it, it creates the exact problem you describe, people are not locked on the same versions and therefore I experience this error.

The lock file should be committed so that we can ensure all contributors are developing with the same versions as each other, and the same versions are used in CI.

Do you not get the type error in your editor? Can you share your lock file (or if it were committed, instead of shared ad hoc, that would be great).

If the lock file is committed, a git pull will update it. If it’s not committed, it won’t. Your explanation seems backwards, respectfully.

@orta
Copy link
Contributor

orta commented Jul 15, 2019

I've just ran through those instructions without problems:

git clone git@github.com:microsoft/TypeScript.git ts2
cd ts2
npm install
gulp
code .

Screen Shot 2019-07-15 at 11 05 00 AM

This project doesn't generate a lockfile in either npm or yarn (#26519 has the reasoning) - so I'm afraid there's nothing I can share from my local setups.

Is it possible you have an old lockfile or npm/yarn settings that are influencing the downloaded dependencies? Maybe npm link?

@joshribakoff
Copy link
Author

joshribakoff commented Jul 16, 2019

This project doesn't generate a lockfile in either npm or yarn (#26519 has the reasoning) - so I'm afraid there's nothing I can share from my local setups.

You could have enabled the lock file to be generated, and shared that, right?

After doing a git pull, rm -fr node_modules, npm i I have 9 errors in checker.ts, but in different places in the code now.

As for the reasoning you have shared for not committing the lock file, I don't think I follow...

Our lockfile is unused in all our CI processes. It also shouldn't be used, since our goal is to always be built with our latest dependencies

But why is the goal to build with latest? If some 3rd party dependency pushes a breaking change, things will break in CI, and on contributor's local machines, right?

Lockfiles are more for reproducible builds using npm ci for end-user applications.

I don't follow the reasoning here for why you wouldn't want reproducible builds? What does it matter if its an end-user application or not? Wouldn't you want reproducible builds either way??

It's presence frequently adds noise to PRs (where updates to it are frequently included by chance)

I don't follow how yarn.lock is updated "by chance"? I bet that stems from your lack of instructions (respectfully). Its a common misconception that npm i uses the lock file like with yarn, but it's not the case unless you use npm ci. If devs were running npm i, it would pull in new versions all the time & churn the lock file, making the lock file seem rather pointless. You also characterize it as noise, but I disagree, if someone built the code using an older version of a dependency, that is very much relevant to the PR reviewer, right? I think the right solution is to switch to yarn or otherwise include instructions to use npm ci, either of these would eliminate accidental churn. Removing the lock file just makes things more erratic & doesn't solve the issue.

its presence is currently flagging the repo with a vulnerability warning because it contains a hard reference to an exact package version with a vulnerability, because it is not actually purposely maintained.

Also don't follow the logic here. The old version of a dependency has a vulnerability? Why not upgrade it? Or does removing the lock file simply suppress the security warning? If so that seems even more egregious?

Bottom line, with both yarn & npm, the lock file is needed to get consistent installs, because it is not included, there isn't consistency.

Is it possible you have an old lockfile or npm/yarn settings that are influencing the downloaded dependencies? Maybe npm link?

I have no lock file present, and no symlinks present.

@RyanCavanaugh
Copy link
Member

@joshribakoff sorry to hear about the problems you're encountering -- we get a lot of PRs from non-team contributors, and this is the first we've heard in terms of setup difficulties, so it seems there's something specific to your machine or configuration that's causing issues. We're not able to provide extensive 1:1 support in terms of getting your editor environment up and running.

If something does end up broken in a way that makes it impossible to get set up, we'll certainly notice it and get it fixed, but it seems like this is an isolated issue of some sort.

@joshribakoff
Copy link
Author

joshribakoff commented Jul 17, 2019

I’m not asking for extensive one on one support. Just reproducible builds. Not sure what else to say really. I guess I won’t be able to contribute.

It’s not specific to my machine. Clearly the issue is that the builds are not reproducible, due to missing lock file.

I have never seen any open source project not commit the lock file, this is a bizarre stance to take for sure.

@tjenkinson
Copy link
Contributor

I hadn't made any changes for a while so ran npm install to bring my self up to date with the package.json but it was broken. Then I ran npm ci which didn't work because there was no lock file which led me here. Clearing node_modules and then npm install fixed it.

It also seems strange to me that there is no lockfile. The reasons in #26519 don't outweigh the advantage of a reproducible build to me. There are tools that exist now to automate bumping dependencies when there are security issues found, or when there are new major versions. Fixing a lockfile when there are merge conflicts is also relatively easy now with npm i --package-lock-only

If a dependency on latest released a new major version wouldn't you want to have the tests run first and be notified about it, and then be able to make any of the changes needed for the new version in sync with updating the version, without there being a period of time where the build is broken?

By having locked versions you also reduce the risk of someone using a malicious version of a package. My thinking is that if a package did get hacked, hopefully this would be discovered before the next time the package-lock needed to be updated. If by chance you did update the package-lock to a vulnerable version of something isn't it better to find out all developers are running a malicious version and then force the update, than not know if the current version is malicious and have some developers effected unknowingly depending on when they last ran npm install?

It's possible I'm missing something here and there are more reasons it doesn't work for you. I just wanted to put my thoughts somewhere.

@joshribakoff
Copy link
Author

@tjenkinson reproducible builds are for wussies! Real developers intentionally introduce non determinism into their builds! /sarcasm

@phiresky
Copy link

phiresky commented May 23, 2020

Just want to add here that this is also really annoying for doing things like git bisect (e.g. #38235 (comment)) to find regressions, since there's no way to build the ts compiler at a specific historical commit unless you are lucky and the dependencies still work.

Edit: Just saw that the official solution to building old TS versions is to bruteforce dependencies until it works. ???

@tjenkinson
Copy link
Contributor

Just spotted #40146 🎉

Still lots of latest versions, but good to see the actual versions are in the history now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

6 participants