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

Competing lockfiles create poor UX #5654

Closed
jmorrell opened this issue Apr 11, 2018 · 93 comments
Closed

Competing lockfiles create poor UX #5654

jmorrell opened this issue Apr 11, 2018 · 93 comments

Comments

@jmorrell
Copy link
Contributor

jmorrell commented Apr 11, 2018

NB: I'm creating this issue in the yarn repo, but this is actually a shared issue between yarn and npm.

With the release of npm 5 back in May, the Node ecosystem now has two lockfile-based package managers. This was overall a win for users, and it has been good to see competition in this space.

However now that there are two competing lockfile formats this can create a new problem for users, especially beginning developers.

When npm 5 came out, Heroku added a new failure if an application was submitted with both npm and yarn lockfiles. In the past few months this has become the most common reason Node builds fail on Heroku and these failures account for ~10-12% of all Node build failures on the platform. Thousands of developers hit this every month.

It's surprisingly easy to end up with both in your repository. Even as an experienced developer I've found myself running the wrong tool for a specific project and having to catch myself before commiting. For an inexperienced developer building their first server / react / angular project who might not understand what a package manager, lockfile, or git repo is, this can be highly confusing.

Neither tool will give a warning or error if the other lockfile exists:

❯ ls *lock*   
ls: *lock*: No such file or directory
                                                                                                                                                         
❯ npm --version
5.8.0
                                                                                                                                                         
❯ yarn --version
1.5.1

❯ npm install
npm notice created a lockfile as package-lock.json. You should commit this file.

added 659 packages from 437 contributors in 10.553s
                                                                                                                                                         
❯ yarn install  
yarn install v1.5.1
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
✨  Done in 7.67s.
                                                                                                                                                         
❯ ls *lock*          
package-lock.json yarn.lock

This is likely especially true for Yarn users where most of the documentation on the web instructs users to npm install. Users who copy + paste commands from docs or Stack Overflow are likely to end up here.

I've spoken to @zkat and @iarna at npm and @arcanis on yarn, and all agreed that this is an issue that should be addressed, but there was not full agreement on how. Ideally this issue prompts discussion and both tools can agree on how we can help users here.

I've compiled a list of potential mitigations that have been suggested to me:

Potential solutions

Do nothing

Is there a technical reason a user might want two lockfiles? In this case, how can external tools determine which package manager should be used for that application?

Error if the other lockfile exists

Yarn could print an error and exit if package-lock.json exists and vice-versa.

Pros:

  • simple and easy to implement
  • the user gets an error where they are in a position to immediately fix it

Cons:

  • Not a fantastic user experience

Convert the other lockfile

Yarn could read package-lock.json, convert it into yarn.lock, and remove package-lock.json. npm could do the opposite.

Pros:

  • great user experience
  • users switching tools would not get a new set of dependencies as a side-effect

Cons:

  • My understanding is that due to the different dependency resolution strategies this conversion would be lossy both ways
  • Requires each tool to add and maintain code to understand the other lockfile
  • Lockfile formats may change over time

Delete the other's lockfile

Just remove the other lockfile and create a new one

Pros:

  • effectively prevents this situation

Cons:

  • surprising behavior
  • user gets new set of dependencies

Run the other tool for the user

If yarn sees a package-lock.json but not a yarn.lock it could log a warning and call npm install for the user

Pros:

  • User gets what they wanted

Cons:

  • Surprising behavior
  • somewhat complicated

Add config to package.json to indicate

Add a field in package.json to indicate which package manager a project should use

"package-manager": "yarn"

Pros:

  • Explicitly conveys the user's intent

Cons:

  • Adds more config for the user

Other?

Maybe I'm missing something that would work better

@ghost ghost assigned arcanis Apr 11, 2018
@ghost ghost added the triaged label Apr 11, 2018
@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

Add config to package.json to indicate

It could be a good use case for the engine field 🤔

@iarna
Copy link
Contributor

iarna commented Apr 12, 2018

Round tripping package-lock.jsonyarn.lockpackage-lock.json is lossy, but that's likely unimportant. yarn.lockpackage-lock.jsonyarn.lock should not be lossy, AFAIK.

From the npm point of view, I favor the option where if yarn sees a package-lock.json and not a yarn.lock that it imports it and removes the package-lock.json. And similarly, if npm sees a yarn.lock and no package-lock.json it should do the same, importing and removing the `yarn.lock.

This would allow users of both tools to seamlessly switch back and forth without leaving the user's project in a confusing state (where files appear to be from both).

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

I'm a bit concerned with this, because it means that neither package-lock.json nor yarn.lock will be able to add metadata to the files (Yarn currently doesn't, but why not), removing some freedom to our formats in the process.

This issue also raises the question of interoperability between Yarn and npm in general, originally discussed in another thread - I feel like trying to be too interoperable might deserve both of us, since users will then have the expectation that everything will work exactly the same way on both project, which I feel is a dangerous assumption (one obvious example is that workspaces will silently break if someone run npm on a workspace-powered project).

@yarnpkg/core, thoughts?

@bestander
Copy link
Member

bestander commented Apr 12, 2018 via email

@BYK
Copy link
Member

BYK commented Apr 12, 2018

Pinging @imsnif since he has expressed interest in implementing the "Convert the other lockfile" solution a few weeks ago. He may even have some working code by now.

@imsnif
Copy link
Member

imsnif commented Apr 12, 2018

Thanks for the ping!

So yes, I'm actually in the midst of working on exactly this. I was planning on writing up a yarn RFC and pinging all stake holders next week.

So briefly: I've done some work on converting yarn.lock <==> package-lock.json. There are some losses in the process, but very little of them are logical. In my eyes, this is totally acceptable if we're talking about the 'one time conversion' scenario mentioned above. If we'd like to discuss this further, I can go into more details.

I currently have some rudimentary code that does this: https://github.com/imsnif/synp
It's not 100% and relies on an existing and current node_modules folder for the project to be converted. I'm in the finishing stages of a re-write (on branch 2.0.0) that will get package state from the registry (using the awesome pacote lib).

What I want to do once that is done is start talking about how (and if?) we'd like to implement this exactly in both yarn and npm.

Would love to hear everyone's thoughts on this.

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

In my eyes, this is totally acceptable if we're talking about the 'one time conversion' scenario mentioned above

I don't think it will be that common. I might be wrong, but I feel like the more common use case will be projects using Yarn, and one of the developer copies/pastes the command from a README to add a dependency, using npm instead of yarn in the process.

In this context, I'm still not convinced it's a good thing to lose data and change the package layout silently by trying to do the right thing - they will probably not even notice it until things break (we could have a confirmation prompt, but I'm expecting people that copy/paste those commands to also blindly confirm install prompts). Worse, it could work on their own machine, but break on their collegues' ones once they switch back to Yarn.

@imsnif
Copy link
Member

imsnif commented Apr 12, 2018

@arcanis - I think all the scenarios and edge-cases are very much worth discussing. In the scenario you mentioned I very much agree, but I think there are other scenarios where there should be a clear bias toward one lockfile.

I see this feature as mainly being used in CI environments, where I think it can shine (as OP mentioned). In the very least though, I think both tools should be aware of the other tool's lockfile? Leaving the specifics of when they decide to make the conversion up to them. What do you think?

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

I see this feature as mainly being used in CI environments, where I think it can shine (as OP mentioned).

Do you have an example? CI systems are supposed to act in a very deterministic way, accidentally commiting a converted lockfile is something that should be caught at worse at PR-time imo, CI is too late for this.

In the very least though, I think both tools should be aware of the other tool's lockfile?

Maybe. Using the engine version to force a specific package manager looks cleaner to me, both projects would just have to keep a list of exclusive engines (like "yarn" cannot be satisfied on "npm", and inversely).

In my mind, converting the lockfiles on the fly also isn't very scalable. For example, we've been only talking about the package-lock.json file until now, but I think pnpm has its own lockfile called shrinkwrap.yaml. Would be nice of we could find a way to deal with both of them and any other format that might come up with one stone.

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

I see this feature as mainly being used in CI environments, where I think it can shine (as OP mentioned).

Also note that if I understood correctly, npm is currently working on a CI-specific very light package manager, which might not work very well with things requiring heavy business logic such as a lockfile converter.

@imsnif
Copy link
Member

imsnif commented Apr 12, 2018

Regarding CI:
My initial thought was: warn when adding a new package with the opposite tool, convert when doing a fresh install (or more conservatively: fail by default on a fresh install and verbosely allow conversion through an import command or config flag for install).

When I previously worked in a mixed npm/yarn environment, we would essentially do this manually with git histories. It was a painstaking process and I'm sure we were/are not alone in this.

As for scalability:
I'd like to account for this as well.
When the conversion is performed, an intermediary logical and physical package tree (if the lockfile has one) are created. When converting to the desired lockfile, those trees are used (or created with sane defaults in case we're converting from yarn and do not have a physical tree).
This way, we can easily convert between other lockfile types as (all we need to do is provide conversions to/from a logical/physical tree for each type).

This is obviously a little ahead of the tool's capabilities right now, but I do not see implementing this as a big issue.

@imsnif
Copy link
Member

imsnif commented Apr 12, 2018

(as for npm's new ci tool - I see what you mean, but I think this is a little ahead of the current discussion?)

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

or more conservatively: fail by default on a fresh install and verbosely allow conversion through an import command

Yep, I'd be totally for adding this conversion logic into the yarn import command, it seems like a useful feature that can be extracted into a separate package if/when we get to implement Yarn plugins :) My comments are focusing on the default behavior we should adopt.

@zkat
Copy link
Contributor

zkat commented Apr 12, 2018

As of npm@6, you should be able to convert package-lock.json -> yarn.lock losslessly, without having to run an install first. It should contain all the data you need, identifiers, etc (npm@6's pkglock has version ranges in the requires field, which matches what yarn.lock uses?)

@zkat
Copy link
Contributor

zkat commented Apr 12, 2018

Oh, and you also need #5042 to be shipped before this is true, as well, since npm doesn't have the sha1 in the pkglock in many cases.

@imsnif
Copy link
Member

imsnif commented Apr 12, 2018

@arcanis - fantastic! So would a PR with:

  1. A warning when installing and finding a package-lock.json file
  2. A warning when adding and finding a package-lock.json file
  3. Adding the ability to convert from package-lock.json through the import command (and deleting the file thereafter)
    Be acceptable? (just so we have something we can use to start the conversation on specifics)

@zkat - that's great about npm@6! If it is up to me, I'd opt for using that and falling back to manifest files in order to support all lockfile versions. Could I ask if you'd be open to implementing similar (or maybe somewhat different) behaviour for npm?

@iarna
Copy link
Contributor

iarna commented Apr 12, 2018

Also note that if I understood correctly, npm is currently working on a CI-specific very light package manager, which might not work very well with things requiring heavy business logic such as a lockfile converter.

If by "currently working on" you mean "have shipped" then yes. =)

As you suggest, currently loading yarn.locks is beyond its scope (as it doesn't know how to layout package trees) but if we all end up with a library that can do that layout I'm not opposed to having it support loading yarn.locks. (Ideally this would be a library shared between it and Yarn.) Obviously for its case it shouldn't remove the existing lock file. It's purely read-only as far as package metadata goes.

A warning when installing and finding a package-lock.json file

I'm concerned that just having warnings when the an unsupported kind of lock file exists won't help with the problems raised by @jmorrell that Heroku is having?

@jmorrell
Copy link
Contributor Author

I'm concerned that just having warnings when the an unsupported kind of lock file exists won't help with the problems raised by @jmorrell that Heroku is having?

A warning would be an improvement, but an error with good messaging would be ideal for users imo. I can only speak for myself, but I have several projects, some that use yarn and some that use npm, and I often find myself typing the wrong one for that project. A quick error means that I can immediately use the one I should have from the beginning.

If it's a warning, it should be big and obvious. IME users are pretty trained to expect to ignore the output of their package manager as long as it worked.

I've reached out to some people who have more experience with beginning developers who will hopefully chime in on what that experience looks like to someone who's just getting started.

CI systems are supposed to act in a very deterministic way, accidentally commiting a converted lockfile is something that should be caught at worse at PR-time imo, CI is too late for this.

I agree. Ideally by the time it arrives at the build / CI service the application is not in an indeterminate state. The only way I would feel comfortable not failing the build in this case was if there was an accepted way for the user to indicate their preference in package.json, but that solution also puts more burden on the user.

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

I'm concerned that just having warnings when the an unsupported kind of lock file exists won't help with the problems raised by @jmorrell that Heroku is having?

It would help them by warning the users that they're doing something they probably don't want to before they actually push on Heroku. I'm not sure how much it would actually change the numbers, but that's something we can just check in a few weeks and see if we need to go further. And since the required changes are relatively small, that might be a good first iteration.

@jmorrell
Copy link
Contributor Author

Honest question for those proposing a warning: Is there a situation where running yarn install or yarn add while a package-lock.json is present would not be a mistake? Ditto for npm install and yarn.lock

@arcanis
Copy link
Member

arcanis commented Apr 12, 2018

Migrating from npm to Yarn, or from Yarn to npm, comes to mind. I'd prefer to avoid adding friction when one wants to try out other package managers.

One thing to also keep in mind is that warnings and errors are not mutually exclusive, or at least not with the engine field. Projects without pinned package managers could print a warning, and projects with a pinned package manager could error. This way users could freely switch from one package manager to the other, until they make their choice and configure their package.json accordingly.

Another advantage of the engine field is that it would allow you to know for sure which version of the package manager has to be used. I guess it's configurable somewhere, but with this system it would be part of the regular workflow.

@jmorrell
Copy link
Contributor Author

jmorrell commented Apr 12, 2018

Another advantage of the engine field is that it would allow you to know for sure which version of the package manager has to be used. I guess it's configurable somewhere, but with this system it would be part of the regular workflow.

Is there an engine field or are you referring to engines? I can't find any docs referring to engine. Apologies if I'm missing something.

We support specifying versions in the engines field: https://devcenter.heroku.com/articles/nodejs-support#specifying-an-npm-version

Ex:

  "engines": {
    "npm": "5.6.x"
  }

There are a couple concerns that I have with using this to determine which package manager to use:

  • The way things look today: a non-trivial number of users have both npm and yarn defined, or an npm version, but then they have a yarn.lock. Future versions of npm and yarn could enforce this, but users who don't upgrade frequently will continue to use current versions for a long time.

  • Most users never touch this config. They forked an old boilerplate that specified Node 0.10 even though they use whatever they downloaded off of nodejs.org on their machine. This causes a non-trivial number of issues on Heroku.

  • Teams often do not coordinate which versions they have installed. The version of Node or npm or yarn they have installed is often whatever was current when they started on a project or the last time one of them broke and forced them to reinstall. Not that this is ideal, but my understanding is that the majority of npm / yarn users are beginners, and this adds a new hurdle to getting started.

Ex: "I cloned the example hello-world project, downloaded Node from https://nodejs.org, and it didn't work"

All that said, I would love, love, love if the tools enforced strict versions and recorded it in engines. It would prevent many of the errors that I see every day, but that feels like a much, much larger change.

@BYK
Copy link
Member

BYK commented Apr 13, 2018

All that said, I would love, love, love if the tools enforced strict versions and recorded it in engines. It would prevent many of the errors that I see every day, but that feels like a much, much larger change.

AFAIK Yarn enforces this field strictly unless you pass a flag to disable that.

Teams often do not coordinate which versions they have installed. The version of Node or npm or yarn they have installed is often whatever was current when they started on a project or the last time one of them broke and forced them to reinstall.

This is quite dangerous, especially when using Yarn since it only guarantees consistency across the same major versions of Yarn.

Not that this is ideal, but my understanding is that the majority of npm / yarn users are beginners, and this adds a new hurdle to getting started.

How about Yarn (and also npm, if they like the idea) adding a "yarn": "^<current_version>" entry into the engines field to help introduce some safety without too much friction? We can automatically add this when running yarn init, yarn import or when we create a lockfile from scratch.

@imsnif
Copy link
Member

imsnif commented Jul 25, 2018

Psst, @arcanis - #5920

@arcanis
Copy link
Member

arcanis commented Jul 25, 2018

Awesome 😍

Between this and your work on yarn import, can we close this issue then?

@imsnif
Copy link
Member

imsnif commented Jul 25, 2018

I thought we'd wait for @jmorrell to give us some stats of how this affects the problem on Heroku so that we can decide if this is enough or we'd like to change something (warning/error, etc.) wdyt?

EDIT: afaik the warning has not been released yet, so we still have some time to wait.

@Spongman
Copy link

Spongman commented Jul 25, 2018

I'm afraid that's a misconception as well.

How so? Volume of traffic at npm tells you nothing about whether or not the project is open source.

a more accurate assessment would be to count which github projects have 0,1 or 2 of (yarn.lock, package-lock.json). personally i have never seen an open-source project (of any appreciable size) on github that has a yarn.lock file and no package-lock.json (except the obvious).

IMO if you're going to keep separate lock files, then BOTH package managers should detect the other's lockfile and ERROR (possibly with an override flag).

@imsnif
Copy link
Member

imsnif commented Jul 25, 2018

All - I'd kindly ask that we stay on topic and take any non-directly-related comments offline (eg. our Discord channel). There are a lot of people subscribed to this thread and in fairness, I feel the npm<>yarn discussion does not belong here. Thanks!

@rzr
Copy link

rzr commented Aug 22, 2018

I havent read the whole tread, but IHMO user should be at least notified that there is an ambiguity:

if npm sees a yarn.lock file then npm should print something like:
"WARNING: this project seems to use yarn, maybe you should use 'yarn' instead of 'npm install'"

if yarn sees package-lock.json file then yarn should print something like:
"WARNING: this project seems to use npm, maybe you should use 'npm install' instead of 'yarn'"

And as suggested above, a way to "prefer" package manager (in package.json).

rzr added a commit to TizenTeam/gateway that referenced this issue Aug 22, 2018
I observed a regression caused to dependencies updates,
build is file but you can't use UI to add things
(browser complains on module.export = App)
Probably caused by babel update from beta49 to rc2 (TBC).

Project prefers yarn over npm,
but npm install could be used by developers or scripts.

Note that, Adding a (potentially unaligned) lock file,
could create ambiguity
but IMHO it's better to support both than only yarn.

Related links:

https://stackoverflow.com/questions/44552348/should-i-commit-yarn-lock-and-package-lock-json-files

yarnpkg/yarn#5654 (comment)

Change-Id: I9489e365b6d3a70102a7d2ae1e44feb7aeb28c06
Signed-off-by: Philippe Coval <p.coval@samsung.com>
hobinjk pushed a commit to WebThingsIO/gateway that referenced this issue Aug 22, 2018
I observed a regression caused to dependencies updates,
build is file but you can't use UI to add things
(browser complains on module.export = App)
Probably caused by babel update from beta49 to rc2 (TBC).

Project prefers yarn over npm,
but npm install could be used by developers or scripts.

Note that, Adding a (potentially unaligned) lock file,
could create ambiguity
but IMHO it's better to support both than only yarn.

Related links:

https://stackoverflow.com/questions/44552348/should-i-commit-yarn-lock-and-package-lock-json-files

yarnpkg/yarn#5654 (comment)

Change-Id: I9489e365b6d3a70102a7d2ae1e44feb7aeb28c06
Signed-off-by: Philippe Coval <p.coval@samsung.com>
jacobq referenced this issue in adopted-ember-addons/ember-electron Aug 30, 2018
@hsribei
Copy link

hsribei commented Aug 31, 2018

I can say that it was perfectly clear to all developers involved back then that the choice of yarn/npm was a per-repo choice, just like the choice of express/hapi, mobx/redux, etc.

This was not clear to me at all. Up until now I thought it was a developer-local choice, and multiple developers could coexist on a single repo using whichever one they liked, while it being a little more convenient to use only one consistently. The express/hapi etc examples are good though and make it clear that that's not a choice. This should have more visibility.

Add a field in package.json to indicate which package manager a project should use

"package-manager": "yarn"

I think this is the best solution, iff it's implemented in all package managers.

The package managers then MUST 100% refuse to proceed if they're called on the wrong project, just as you would expect an error if you required redux but then tried calling mobx functions on it. They should issue an error, not a warning, and inform the user how to proceed with the correct package manager.

@iarna
Copy link
Contributor

iarna commented Aug 31, 2018

Add a field in package.json to indicate which package manager a project should use
"package-manager": "yarn"

I think this is the best solution, iff it's implemented in all package managers.

If you want npm to consider this I encourage you to open a discussion in the appropriate venue, but I'll say that I don't like this direction. I would like to see convergence between package managers not divergence.

@jasonkarns
Copy link

I would recommend against some new "package-manager" field, instead recommending the existing engines stanza which already has prior art.

Both the yarn and npm docs mention (in passing) usage of engines.npm and engines.yarn to specify the versions of each that are expected to be used. Yarn will even error out if the yarn version doesn't satisfy engines.yarn.

https://yarnpkg.com/en/docs/package-json#engines-
https://docs.npmjs.com/files/package.json#engines

Even if neither npm or yarn check engines for the "competing" manager (which would be nice), using this field is still a great way to communicate to other developers which manager is expected to be used. (if the existance of package-lock.json or yarn.lock isn't enough of a clue)

@Spongman
Copy link

Spongman commented Aug 31, 2018

I would like to see convergence between package managers not divergence.

Agreed. Either convergence, or refusal.

but the current ambiguous, erroneous middle-ground is harmful.

I still think that just throwing an error in the presence of the others' lockfile would require the least friction for existing projects.

@hsribei
Copy link

hsribei commented Aug 31, 2018

If you want npm to consider this I encourage you to open a discussion in the appropriate venue

Thank you, I've just signed up. There's a recent thread in this direction: https://npm.community/t/npm-install-should-warn-about-the-presence-of-yarn-lock-files/1822

My vote is for erroring out full-stop instead of just warning, so not sure if I should append to that thread of start a new one.

I would like to see convergence between package managers not divergence.

Same here. Though they are incompatible right now, so the divergence has already happened and is already causing friction to many users and projects.

Whatever convergence comes out of further talks amongst the teams is going to take time. When it does arrive, the proposed errors can be lifted and users can start switching and mixing package managers without a cinch.

Adding the errors now avoids further confusion and doesn't stop efforts in the direction of convergence.

I would recommend against some new "package-manager" field, instead recommending the existing engines stanza which already has prior art.

That sounds reasonable. I'm not attached to any particular method of detecting the presence of an incompatible package manager.


To add illustration, my use case in particular is gatsbyjs. The situation is the exact same as reported by @gaearon here:

To Create React App users, this is extremely confusing because their project gets created with Yarn (if Yarn exists on the system) but then they follow the docs of some library that tells them to npm install, and that blows away the whole tree.

To avoid this problem, even though gatsby uses yarn, it also adds a package-lock.json to default starters. But then users end up with both lock files and start seeing warnings. It's easy to just delete one of them, but it muddles up the onboarding experience.

@imsnif
Copy link
Member

imsnif commented Aug 31, 2018

Thanks everyone for your input.

@jmorrell - #5920 was released in 1.9.2 on Jul 25. Been a little over a month, I know it's not a lot of time (and surely not everyone has upgraded) - but do you maybe have some insights to share regarding the double-lockfile errors on Heroku since then?

@jmorrell
Copy link
Contributor Author

jmorrell commented Sep 19, 2018

@imsnif Thanks for the email reminder! Sorry for missing the update here.

Here is a graph of the number of apps that experienced this particular failure each week in 2018.

(I've redacted the y scale, but this represents 100's of developers and shows the trend)

two-lockfile failures

  1. The dip in July comes from an unrelated issue with S3

  2. Looking at the trend after July 25, the warning seems to have had a substantial effect on the number of people experiencing this error. I am somewhat surprised by the magnitude of the shift.

@imsnif
Copy link
Member

imsnif commented Sep 19, 2018

@jmorrell - this is really great!
I'll admit I'm also quite surprised at this.

I think (on the yarn side) this issue can be considered resolved. Do you agree?

@Spongman
Copy link

Spongman commented Sep 19, 2018

i still think the advice of removing package-lock.json to fix the error is misleading - this will in general result in a different set of package versions being used, and will (again, in general) lead to incompatibilities and breakages. also, this advice is only useful for those who make decisions about which package manager a project is using. general contributors following this advice are also going to run in to issues.

IMO if a package-lock.json exists and yarn can't guarantee that the versions it installs are the same as those that npm would install, then it should fail with an error.

@imsnif
Copy link
Member

imsnif commented Sep 20, 2018

Thanks again for your input @Spongman. I feel this has all been well discussed in the (very long) thread above us and I don't think it makes sense to restart it again. I think we all understand your position. Thanks for participating.

Unless @jmorrell tells me otherwise in the next week or so, I'm going to consider this done and close this issue.

@jmorrell
Copy link
Contributor Author

@imsnif Taking a different track and looking at failures from September so far (Sep 1 - Sep 20), this is still the number 1 reason that Node builds on Heroku fail. It happens twice as much as the next most common known failure case: package.json being invalid JSON. Once again redacting exact numbers, here is how it stacks up to other issues.

node build failures september 1 - 20 2018

I haven't seen any movement from npm on this, so I'll create a PR adding a warning there too and see it gets accepted.

I think (on the yarn side) this issue can be considered resolved. Do you agree?

I don't think this has been resolved for users, and the user experience still isn't great. But I can see how the numbers change once npm also gives feedback to a user hitting this case.

I'll leave it up to you whether you want to close this or make further changes. I'm happy to open a new issue in a few months if this is still a big problem for users.

@imsnif
Copy link
Member

imsnif commented Sep 20, 2018

@jmorrell - seeing as this issue is attracting a lot of attention and the conversation keeps restarting and even repeating itself, I think it would be better to close it now with the information we have.

If, with npm providing a warning as well, we still see an issue, I'd definitely be willing to start the error discussion again (or look into other solutions). Please, do ping me personally in that case.

Thanks for bringing this up, contributing and making all these changes happen! I personally feel yarn is better after this issue.

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

No branches or pull requests