yarnpkg / yarn

The 1.x line is frozen - features and bugfixes now happen on https://github.com/yarnpkg/berry
https://classic.yarnpkg.com
Other
41.39k stars 2.73k forks source link

Competing lockfiles create poor UX #5654

Closed jmorrell closed 6 years ago

jmorrell commented 6 years ago

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:

Cons:

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:

Cons:

Delete the other's lockfile

Just remove the other lockfile and create a new one

Pros:

Cons:

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:

Cons:

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:

Cons:

Other?

Maybe I'm missing something that would work better

arcanis commented 6 years ago

Add config to package.json to indicate

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

iarna commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I like @iarna’s idea to make both tools migrate from one lock file to another on install automatically. Makes sense to delete the imported lockfile after the migration to avoid both package managers competition in one project.

Both tools could print warnings and maybe reqire user prompt to proceed.

I also agree with Mael’s point that achieving 100% compatibility will lock us from exploring new features, I think we should treat it as a one-off migration path, that could be a rather small feature in install.js on our side.

On Wed, Apr 11, 2018 at 9:49 PM Maël Nison notifications@github.com wrote:

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 https://github.com/orgs/yarnpkg/teams/core, thoughts?

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/yarnpkg/yarn/issues/5654#issuecomment-380677110, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBdWI9jnLJeFqH8v2T-AB74sQO1PMIjks5tntzrgaJpZM4TQ5-B .

BYK commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

(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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Oh, and you also need https://github.com/yarnpkg/yarn/pull/5042 to be shipped before this is true, as well, since npm doesn't have the sha1 in the pkglock in many cases.

imsnif commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

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.

thatlittlegit commented 6 years ago

I don't know much about Yarn, but I think the best solution would be to add a package-lock.json parser into Yarn and don't create a yarn.lock if it exists.

I think that the end goal should be to have one lockfile format shared among Yarn and npm.

iarna commented 6 years ago

@thatlittlegit You'll be pleased to know about https://github.com/yarnpkg/yarn/pull/5745 then. (npm will be doing the same in reverse, so we'll end up in a place where it doesn't matter which kind of lock file you're working from or which tool you pick.)

Edited to add: A single lockfile format is not, I think, a likely outcome as there are different tradeoffs between the lockfile formats and lack of consensus as to which one's are best. I think having both tools being able to mutually understand one another's lockfiles is sufficient.

Beyond that, there are a ton of advantages of having multiple actively used tools that make different tradeoffs because it means user use cases can be better met than can be with a single general solution.

iarna commented 6 years ago

@BYK Some history on the behavior of the engine field would probably be helpful here:

In npm@1 and npm@2, we had a package.json called engineStrict in addition to the engine fields. If engineStrict were true then the engine field was used as resolution constraint and before we applied the semver range to the list of versions, versions with incompatible engines would be filtered out. This would let you do things like npm install foo and get foo@1.0.0 even if foo@2.0.0 was published if foo@2.0.0 wasn't supported on your platform. This seems desirable, but in practice was super confusing, particularly because docs on the website were only for the most recent version.

If engineStrict were false then resolution was done without regard to the engine fields and warnings were issued if the result wasn't compatible.

There was ALSO an engine-strict config option, which did the same thing as the package.json property, but applied it to ALL packages.

Starting in npm@3, we dropped support for the engineStrict package.json property and changed the behavior of the engine-strict config option. The config option now turns the warnings that you get with it off into errors, but does not impact resolution.

The engine fields are enforced not just for the top level project but transitively. Introducing a yarn engine flag would imply that dependencies can only be installed with yarn and I don't think that's the case here.

imsnif commented 6 years ago

Since in the next yarn release we should have the ability to import package-lock.json to yarn.lock - I'd like to go back to discussing the warning/error issue.

The way I see it: I don't think there's a good reason for a single package to have both a package-lock.json and a yarn.lock file. Most projects I've seen who have this knowingly view it as an issue that needs to be fixed rather than a desired situation (but I'm of course open to be corrected). As I understand from @iarna's explanation above, the engine field won't be a good solution to explicitly specify which package manager a package is using. So IMO, this is a situation that should produce a verbose error that would invite the user to either delete one file or convert it.

However, I think producing such an error would be a clear breaking change. On the yarn side, I'd propose starting out with a warning (as mentioned above) and slowly deprecating this behaviour until the next major version in which it would be changed to an error (maybe adding a config flag that would allow both files to exist simultaneously).

What does everyone think? @jmorrell, @BYK, @arcanis, @iarna?

BrainBacon commented 6 years ago

It seems odd to me that any kind of project should specify what package management tool should be used. Correct me if I'm wrong, but I see Yarn as a drop-in replacement that hinges mostly on end user preference. Is Yarn trying to replace NPM entirely as the de facto standard, or is it attempting to coexist?

Vinnl commented 6 years ago

@BrainBacon You only get the benefits of locked dependencies if everyone uses the same package manager that respects the same lockfile and uses the same semantics. If a project has a yarn.lock and someone then runs an npm install, that person could still end up with a different set of dependencies. So no, it's not dependent on end-user preference, unfortunately - preferably, everyone on a project uses the same package manager.

This also means that it does not make sense to have two lockfiles in a project. They are unlikely to resolve to the same dependency tree, and make it unclear to contributors which package manager to use.

(The drop-in replacement part was especially true when npm did not produce lockfiles, as there was no information to get lost there. It's also still true if #5745 really means that Yarn can now produce a lockfile based on package-lock.json, but that only means that a project can then easily replace npm by Yarn. However, since you still need to check in the new lockfile, all contributors will need to switch.)

arcanis commented 6 years ago

Correct me if I'm wrong, but I see Yarn as a drop-in replacement that hinges mostly on end user preference.

Yarn isn't a straight up drop-in replacement in term of exposed API. The yarn add <pkg> vs npm install <pkg> is an obvious example of something where we do things differently. We do tend to have the same feature set, but in the end it's two different tools and sometimes we have different solutions to a same problem.

The yarn.lock vs package-lock.json is one of those differences and I believe neither Yarn nor npm are interested into using a single one, as they contain different information.

On the yarn side, I'd propose starting out with a warning (as mentioned above)

I'd be fine with this. Something like:

WARN Your project seem to contain lock files (package-lock.json, shrinkwrap.json) generated
WARN by other tools than Yarn. It is advised not to mix package managers, in order to avoid
WARN resolution inconsistencies caused by desynchronized lock files.

Does someone want to open a PR?

jmorrell commented 6 years ago

I'd be happy to open a PR that adds a warning :)

Is the desired end-solution here a helpful error when package-lock.json exists + yarn import? My understanding was that @iarna was advocating that each tool should remove / convert the opposite lockfile if it exists automatically.

Either solution would be a significant improvement for users, but I think it would be best if both yarn and npm chose the same strategy (though not strictly necessary if there isn't agreement and people feel strongly about it).

imsnif commented 6 years ago

@jmorrell - my understanding was that we agreed automatic conversion is a bad idea here.

Mostly because it's hard to know what the delta between the two lockfiles should be. Eg. when I see the above warning as a developer, what I'd probably want to do is figure out when the other lockfile was created, what packages were added to it that were not added to my main lockfile and then add them. Maybe using the import option as a reference, but not necessarily.

I hope the npm folks agree on this one?

Ideally, I'd like the warning to include something about this being a deprecated situation (not sure about the wording) and that future yarn versions will produce an error. Maybe linking to some detailed documentation about this and how to address it. @arcanis - do you think it's feasible, or would you rather stay with the warning behaviour?

If both tools issue an error when there are multiple lockfiles, I feel developers will catch on to this issue while it's still "small" and be able to address it quickly (with the help of importing as reference or without).

zkat commented 6 years ago

@arcanis npm add <pkg> is perfectly valid and does what Yarn does 👼

As far as pkglock vs yarnlock, the original intention of package-lock.json was to create a format that included all the data either package manager needed. In fact, pkglock as of npm@6 can be transparently converted into a yarn.lock without executing an installer, since it describes a full tree with all the relevant metadata. We did that intentionally (such as the requires field), and we asked for feedback from both the Yarn and pnpm teams to make sure that whatever we did was sufficient for both of them to use either as an importable lockfile or as an alternative option. yarn.lock is, unfortunately, a lossy subset so the inverse is not true, but we'll be adding stuff to import it anyway. npm would even obey Yarn-calculated tree shapes (unlike Yarn, which would not).

arcanis commented 6 years ago

In fact, pkglock as of npm@6 can be transparently converted into a yarn.lock without executing an installer, since it describes a full tree with all the relevant metadata.

I don't think it can (but maybe I'm mistaken, feel free to point any misconception). The Yarn lockfile doesn't support by design multiple ranges resolving to a single version. If you have multiple packages each depending on lodash@^1.0.0, they will all use the exact same version. It's not only an optimization, but also how we encode things in our lockfile.

The npm lockfile being a tree, this property isn't guaranteed, and multiple identical ranges might end up using different versions (which is fine, since it might allow slightly better optimizations by exploiting the Node resolution rules). In such circumstances, the package-lock.json -> yarn.lock conversion would be lossy, since we wouldn't have any other choice than to merge them into a single one.

On the other hand, a Yarn lockfile can be transformed into a package-lock.json relatively without trouble, because our rules are a stricter subset of yours (our merged ranges can be transformed into a tree without loss, a tree cannot be transformed into a merged range without loss). Note that I'm only talking about the dependency tree itself, I'm not familiar with your metadata fields.

To be honest I'm not entirely sure whether we say the same thing or something entirely different, I just wanted to clarify a bit the situation 🙂

Ideally, I'd like the warning to include something about this being a deprecated situation (not sure about the wording) and that future yarn versions will produce an error. Maybe linking to some detailed documentation about this and how to address it.

@imsnif I'm not convinced about this being an error in non-CI environments (no question about CI - this is something that should definitely trigger an error). I'd prefer to stay on a warning that simply describe the current state, and explain why it's not ideal.

A good thing is that @jmorrell will be able to provide us precise metrics to see whether the warning has been enough, and based on that we'll choose our next move.

Is the desired end-solution here a helpful error when package-lock.json exists + yarn import?

Yep, coupled with the "upgrade-to-error-on-CI" mode* I think it's a reasonable first step!

(* This doesn't exist yet, I'll mention it in the #5773 Yarn 2.0 WG)

imsnif commented 6 years ago

@arcanis - About lockfiles: I think what @zkat is referring to (but please @zkat correct me if I'm wrong or misunderstanding) is that package-lock.json saves the physical as well as logical tree, while yarn.lock only saves the logical tree. So as mentioned earlier, converting package-lock.json => yarn.lock => package-lock.json will lose the physical tree data. IMO in most cases this is fine, but there are exceptions: eg. bundled dependencies.

My personal opinion is that both package managers can benefit from syncing their construction of a physical tree. From what I've seen, we can probably extract some well defined rules about how a physical tree is created and both follow them (ideally with a module maintained by all stakeholders). I think this will both solve the compatibility issue, some issues yarn has with bundled dependencies, while still allowing each package manager to keep its unique quirks. (Though admittedly, I'm quite new to this discussion, so there might be things I'm unaware of - my apologies if I'm waking some sleeping dogs).

To the issue at hand though: @arcanis - I'll try to offer just one more argument here in favour of blanket errors in this situation, and if you disagree we can stay with the warnings as far as I'm concerned: While it is indeed quite important to have this caught on the CI level, I think an error there would be significantly harder to debug (on average) than an error in the developer's local environment. If a developer gets an error on their local environment for using the wrong tool, they (likely) won't go even one step further. They'd just say "oops", and use the other tool. It will save a tremendous amount of time, in place of catching it on the CI after developing a feature and then backtracking. A warning (as was mentioned previously here) might get swallowed up in quite a few others that developers tend to ignore if the exit status is 0. What do you think?

imsnif commented 6 years ago

Also - in the situation I described above the "foreign" lockfile might be understandably placed in .gitignore and so the CI won't even get a chance to know about it. As an absent-minded developer, I might think "Well, it gives me some obscure warning on my local environment, but it passes CI - so it's probably just a local issue on my end - ah well"

arcanis commented 6 years ago

A warning (as was mentioned previously here) might get swallowed up in quite a few others that developers tend to ignore if the exit status is 0.

My point is that this affirmation isn't backed by any data, even though we do have a way to collect some to then make an informed decision. In this context, choosing the more radical option that will break peoples' workflow seems to me a bit futile and potentially harmful 😕

Additionally, I think you're kinda overlooking the user story of "I'm using a package manager and want to try another one", which I think is quite important for everyone involved, both package manager maintainers and users alike. It wouldn't be great if people weren't able to easily try out npm 6 from their Yarn project (or conversely, to try Yarn from their npm project).

My personal opinion is that both package managers can benefit from syncing their construction of a physical tree.

This is a different topic, but I disagree. I think there's a misconception that Yarn is npm, which is incorrect. If you want to import a project into another through a dedicated command I'm all for it, but silently converting (or reading) lockfiles from a third-party format is a recipe for disaster:

imsnif commented 6 years ago

My point is that this affirmation isn't backed by any data, even though we do have a way to collect some to then make an informed decision. In this context, choosing the more radical option that will break peoples' workflow seems to me a bit futile and potentially harmful confused

True - it's not backed by any data. I'm definitely speculating here (sorry if I did not emphasize this). While we might be able to get a sense for the trend of the warning fix, we won't be able to get a sense of how convenient this is to users and whether there is a more convenient solution. If we rely on data, I don't think there is any result other than "there was absolutely no change in the Heroku deploy error rate" that will steer us toward the blanket error solution.

I agree it's quite important not to break people's workflows. Which is why I proposed the error as a breaking change in 2.0.0, with wording in the warning that would alert the user that this situation is deprecated. I also think we can permit the use of a --force or config parameter to override this behaviour. (I believe this also addresses your trying out the other package manager concern?)

This is a different topic, but I disagree. I think there's a misconception that Yarn is npm, which is incorrect. If you want to import a project into another through a dedicated command I'm all for it, but silently converting (or reading) lockfiles from a third-party format is a recipe for disaster

I think you raise important concerns here which I'd love to address. But as you say, this is a different topic and I don't want to derail the conversation. Maybe we can discuss it in a separate issue?

BYK commented 6 years ago

I think the conversation here got derailed a bit so I want to tidy it up with some clarifications and a summary:

  1. Yarn and npm have different resolution and physical tree creation strategies and this is a key factor for both projects to exist.
  2. Both yarn.lock and package-lock.json files were created with specific goals in mind and as far as I can see, these goals don't fully align but have some overlaps.
  3. The difference in these goals don't make a format superior to other in general, they just trade off different things.

Based on these, I think keeping both formats is actually critical for innovation and serving different needs. What is needed is ensuring project portability with minimal data loss between package managers without imposing a particular package managers strategy onto the other to allow free experimentation.

I see the acknowledgment and warning against other lockfiles along with proper conversion with a clear messaging about what is conserved and what is lost as a great value to users on each side. So now the question here in this issue is what is the best flow for users that have a package-lock file in their repo when they run yarn.

Looks like auto conversion is potentially dangerous and failing installation without a way out may hurt certain people. How about requiring a config to be set for yarn to know that having a package-lock file is intended in CI mode? This is a signal from the developers to yarn, saying "I know what I'm doing so please do not try to protect me against any discrapencies".

Thoughts?

imsnif commented 6 years ago

How about requiring a config to be set for yarn to know that having a package-lock file is intended in CI mode? This is a signal from the developers to yarn, saying "I know what I'm doing so please do not try to protect me against any discrapencies".

@BYK - sounds great. With one addition: as @arcanis convinced me here and on discord, it might be good to also add a warning in non-ci mode when the config flag is not set. So that developers have a chance to know their CI build might fail before they push (and also to protect against package-lock.json being in .gitignore).

zkat commented 6 years ago

@arcanis as @imsnif mentioned: pkglock includes both the logical and physical layout of the tree. By following the logical version of the tree (which includes the ranges that we deduped from), you can construct a yarn.lock in-memory with no installs or running installer logic at all. Just tree lookups :)

ishitatsuyuki commented 6 years ago

From NPM 5 release notes:

A new, standardised lockfile feature meant for cross-package-manager compatibility (package-lock.json)

It seems that the NPM team advertises package-lock.json as the universal solution; is it (technically) possible to join that force?

One solution I'd like to propose would be "use only package-lock.json when it already exists". That's basically how NPM "migrated" from shrinkwrap to a lock json file.

arcanis commented 6 years ago

It seems that the NPM team advertises package-lock.json as the universal solution; is it (technically) possible to join that force?

We already discussed it, please check the rest of the thread if you haven't. This isn't going to happen in the short term future.

arxpoetica commented 6 years ago

I think you're kinda overlooking the user story of "I'm using a package manager and want to try another one", which I think is quite important for everyone involved

This probably isn't a frequent thing, context switching. I picked Yarn a while ago and haven't looked back. I think it's better to throw some weight behind the seriousness of why the lock files are there. Make it an error that can be opted out of, rather than a warning that will be ignored. When someone commits a lock file, it's for a reason.

iarna commented 6 years ago

Our stats show that the majority of Yarn users also use npm. Mixed projects are uncommon, but a single person working on multiple projects, some of which are npm and some of which are Yarn, is quite common. Further complicating the issue, install scripts that run npm directly are a thing.

Spongman commented 6 years ago

I want to use yarn, but most of the projects I work on only have package-lock.json files. I cannot add yarn.lock files. yarn import is slow and/or broken. Currently my only option is to ditch yarn and switch to npm.

I understand that yarn is built by yarn-only developers for yarn-only projects. But what about the rest of us?

imsnif commented 6 years ago

Hey @Spongman - yarn import should not be broken. It should now be able to import package-lock.json for you. If you're having issues please let us know!

Spongman commented 6 years ago

I did #6103

imsnif commented 6 years ago

Hey @Spongman - I commented in the issue, this specific case was due to a corrupted package-lock.json. I'd be happy to know about any other problems.

Spongman commented 6 years ago

npm can use that package-lock.json file just fine. yarn needs to be more resilient.