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

Add yarn to the Travis tests matrix #3554

Closed
wants to merge 4 commits into from
Closed

Add yarn to the Travis tests matrix #3554

wants to merge 4 commits into from

Conversation

aymericbeaumet
Copy link

It has been reported here that installing Hapi with Yarn is not tested on the CI.

This PR adds Yarn to the Travis tests matrix.

This allows us to make sure Hapi can successfully be installed with the two main package managers. This patch is flexible and allows us to add more in the future if need be.

@aymericbeaumet
Copy link
Author

According to Yarn recommendations, one should use apt-get to install Yarn. This is not possible on Travis with the sudo mode disabled. That's why I headed for the npm install mode, even though it's not the recommended way.

@kanongil
Copy link
Contributor

Thanks, but yarn is not compatible with Hapi. It can install the wrong versions of the dependencies due to yarnpkg/yarn#838, which they refuse to fix.

@aymericbeaumet
Copy link
Author

I understand that Hapi needs to provide reproducible builds in some context, but if that's at the cost of being incompatible with other package manages (namely: Yarn), I think we have to work something out.

This can be approached like so:
1/ Hapi should work as-is with the versions provided in the package.json (for both npm and yarn)
2/ npm-shrinkwrap.json should be present as a bonus to ensure the determinism of a build for folks using npm, but it should not break without it

That PR complies with 1/, while still allowing 2/.

@corbinu
Copy link

corbinu commented Jul 25, 2017

I just wanted to say I am -1 on this. It was yarn's choice to do a total rewrite and fork the world yet in many cases they want to break backwards compatibility.

The onus is not on packages to change for yarn but for yarn to make sure it is compatible with the existing packages.

@kanongil
Copy link
Contributor

Why should Hapi change for third-party package managers? IMO, it should be their job to adhere to the way npm installs dependencies.

@nlf
Copy link
Member

nlf commented Jul 25, 2017

I concur. Supporting third party package managers is not our burden. If they happen to work, neat, but we definitely don’t support them.

@aymericbeaumet
Copy link
Author

aymericbeaumet commented Jul 25, 2017

The Yarn team chose to drop support for the npm-shrinkwrap.json format, I'm sure they had good reasons to do so, but I don't think that's really the point here. My point is npm and yarn are theoretically compatible as they both obey to the same source of truth: package.json.

The issue here is deeper than whether Hapi should support third parties package managers. Hapi is performing a non-recommended use of the npm shrinkwrap feature, this was working fine while the whole community relied on npm, but the arrival of an alternative solution enlightened this issue. I don't see it as very healthy to have a package strongly tighten to a single package manager.

As per the npm documentation, lockfiles are not recommended to be used by libraries:

The recommended use-case for npm-shrinkwrap.json is applications deployed through the publishing process on the registry: for example, daemons and command-line tools intended as global installs or devDependencies. It's strongly discouraged for library authors to publish this file, since that would prevent end users from having control over transitive dependency updates.

The final users should have control over their installed dependencies. If need arises to have fine control over dependencies in a library, one could specify a fixed version in the dependencies of the package.json. npm-shrinkwrap.json is not the only way to achieve your goals here.

I don't think I'm the only one using both yarn and hapi. Let's make this work for everyone 👍

@hueniverse
Copy link
Contributor

I appreciate the desire to make hapi easier to use and more compatible with third-party package managers.

However, I am not having this discussion again, not until there is an acceptable solution. Using a shrinkwrap file is the only way to lock down n-order dependencies, not just direct ones. It is also the only solution available today that is compatible with versions of npm distributed with node 4.x.

If there are specific changes we can make to resolve specific issues (like using ^ in package.json to force a newer version), I am open to that. But anything that takes absolute control away from me in deciding each and every approved dependency version is DOA.

@aymericbeaumet
Copy link
Author

But anything that takes absolute control away from me in deciding each and every approved dependency version is DOA.

I get your point and I understand you want to provide a determinist build when you ship to npm. That strong contract brings guarantees and I think that's great.

That said the lockfile should be seen as a bonus, it should not be a requirement. One could use npm without leveraging lockfiles (npm install --no-shrinkwrap), this shouldn't result in errors. SemVer is supposed to give us that level of confidence, and SemVer is supported by both npm and yarn.

My goal with this PR is to ask: Hapi is strongly leveraging the lockfiles, what is happening if we disable it (third party or not)?

@nlf
Copy link
Member

nlf commented Jul 26, 2017

This is also unsupported usage, and anyone doing so and filing issues the first thing they would be told is to use a supported installation method.

I appreciate how passionate and persistent you are in your desire to get this merged, but as has been said - installations that don’t respect the lock file will never be supported. That’s just the facts.

You’re much better off and would have a much bigger and more positive impact on the community by continuing to put pressure on the yarn maintainers to support shrinkwrap files in dependencies and subdependencies appropriately.

I’m closing this for now, if yarn supports shrinkwrap files at a later date we’d be happy to revisit.

@nlf nlf closed this Jul 26, 2017
@aymericbeaumet aymericbeaumet deleted the aymeric/add-yarn-to-ci branch July 26, 2017 11:33
@aymericbeaumet
Copy link
Author

aymericbeaumet commented Jul 26, 2017

I see, then I think people should know that npm (with shrinkwrap) is strongly recommended to install Hapi (as the only officially supported way). Would you be willing to accept a PR providing such a notice in the readme?

Thanks for your input, will try to find time to contribute on that topic 👍

@hueniverse
Copy link
Contributor

@aymericbeaumet I am going to make the package.json file a bit more prescriptive to solve the actual issue. That part is annoying me but I can't excuse not doing it...

We will always take improvements to the documentation.

There is a difference between saying hapi doesn't work with yarn vs. explaining what protection you are losing when you don't use npm.

@aymericbeaumet
Copy link
Author

@hueniverse That's great!

There is a difference between saying hapi doesn't work with yarn vs. explaining what protection you are losing when you don't use npm.

We are on the same line 👌 That's what I tried to express here: outmoded/hapijs.com#498

@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants