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

enforces that contributors use yarn not npm #439

Merged
merged 3 commits into from
Aug 23, 2019

Conversation

dgreene1
Copy link
Collaborator

@dgreene1 dgreene1 commented Aug 23, 2019

This will be the true fix to #438 which recognized that some contributors were getting different errors based on if they were using package-lock.json or yarn.lock. This is what was changed:

  • We will only use yarn.lock
  • package-lock.json was removed and if it comes back then ensureYarn.js will throw and fail the tests (and the commit thanks to Husky)
  • the engines portion of package.json now enforces a specific version of yarn

@dgreene1
Copy link
Collaborator Author

Check this PR out @HoldYourWaffle & @WoH. I'm going to merge it so you guys can try out how it behaves. Note that you will have to either install yarn or upgrade it to the version specified in the "engines" part of package.json

@dgreene1 dgreene1 merged commit 14b1dfe into lukeautry:master Aug 23, 2019
@HoldYourWaffle
Copy link
Contributor

Are you sure you want to enforce yarn instead of npm? Npm is a lot more widespread (and included with node.js), so I don't think it's contributor friendly to outright prohibit it.

Yarn can cooperate with npm's package-lock.json just fine (albeit with a warning), while npm doesn't understand the yarn format at all. In other words: as long as we make sure yarn never actually uses the yarn.lock file (for example by deleting it) we'll never get inconsistent results (since it'll use the repository-provided package-lock.json), meaning there's no reason to outright block using one or the other.

By the way I locked the Typescript version because even minor (or sometimes even patch) versions can (and have done so in the past) break the compiler api. Even with a functioning lockfile this could lead to issues by careless updating. It's more an insurance than an actual solution for a problem though, so if you want to leave it unlocked that's totally fine by me.

@dgreene1
Copy link
Collaborator Author

dgreene1 commented Aug 23, 2019

Are you sure you want to enforce yarn instead of npm? Npm is a lot more widespread (and included with node.js), so I don't think it's contributor friendly to outright prohibit it.

Yes. tsoa always did use yarn. That's what is run in CodeShip. The fact that a package-lock.json was committed was a mistake that should have been caught in a PR review. Now that I have a script in there that throw if one is found, we no longer have to worry about a developer making that mistake again.

Yarn can cooperate with npm's package-lock.json just fine (albeit with a warning), while npm doesn't understand the yarn format at all.

If we're using yarn (which tsoa always has been), then we should use the file that yarn expects.

By the way I locked the Typescript version because even minor (or sometimes even patch) versions can (and have done so in the past) break the compiler api. Even with a functioning lockfile this could lead to issues by careless updating. It's more an insurance than an actual solution for a problem though, so if you want to leave it unlocked that's totally fine by me.

Unless I'm mistaken, I believe we will only have breaks in the compiler api if we go backwards. And the lockfile will ensure that that doesn't happen. As far as moving forward, tsoa should continue to use the latest typescript so that we're not out of sync with what our users are getting with VSCode (which automatically upgrades TypeScript).

@HoldYourWaffle
Copy link
Contributor

If we're using yarn (which tsoa always has been), then we should use the file that yarn expects.

I disagree with your "hard logic" (English is not my first language...) here. Although yarn doesn't specifically "expect" package-lock.json, it's able to handle it perfectly fine. Unless yarn has some incredible feature that I don't know about (in which case I'd be very eager to know), I don't see how a little warning justifies banning the most popular (and bundled) package manager.
This doesn't apply if there's a safety (in the sense of dependency locking) benefit of course, but as far as I know there is none.
"It's what we've always used" doesn't sound like a good argument to me either, since you can continue using yarn just fine with package-lock.json.

Unless I'm mistaken, I believe we will only have breaks in the compiler api if we go backwards

I have unfortunately seen it break even when moving forward :( Again if you want to leave it unlocked that's perfectly fine by me, it's just a suggestion on my end.

@WoH
Copy link
Collaborator

WoH commented Aug 23, 2019

I don't see how a little warning justifies banning the most popular (and bundled) package manager.

The project uses yarn and it should make sure the issues we both noticed do not impact anyone else? I don't see how that's really banning. It's like the linting, in order to make sure the code is consistent, you need to make decisions that every person working on it may not always agree on. I don't think you'd say linter rules are a ban even if they equally are.

@HoldYourWaffle
Copy link
Contributor

I don't see how enforcing a certain "visual style" for the code is the same as "you have to use this specific piece of software, that does exactly the same as the one you're familiar with (and is probably already installed on your system) for no reason other than a little warning for those that use our choice".

The issues we experienced shouldn't happen anymore because yarn supports package-lock.json. Unless there's a difference in how it handles this format (or some other consistency problem I don't know about), I honestly don't see a reason why we'd have to ban npm. If there is such a reason please tell me, because I'm really starting to feel like I'm missing something obvious here 😅

@WoH
Copy link
Collaborator

WoH commented Aug 23, 2019

you have to use this specific piece of software, that does exactly the same as the one you're familiar with (and is probably already installed on your system) for no reason other than a little warning for those that use our choice

-> you have to use this specific type of code format, that does exactly the same as the one you're familiar with (and is probably already what you understand more easily) for no reason other than a little warning for those that use our choice.

@dgreene1
Copy link
Collaborator Author

in order to make sure the code is consistent, you need to make decisions that every person working on it may not always agree on.

Yes, thank you @WoH. I did not take this decision lightly. I did it for one pragmatic reason:

I have no ability to modify the CodeShip configuration since that is owned by Luke (who I am purposely not tagging). I've already asked him to give me access to modify the CodeShip build steps. But since I can not do that yet, we are have no choice but to use yarn.

Although yarn doesn't specifically "expect" package-lock.json, it's able to handle it perfectly fine.

You're making an assumption that yarn works perfectly fine. I would wager that the majority of yarn's automated unit tests verify their interoperability with their first party item (yarn.lock). As the primary maintainer of this library and the npm module, I take a lot of responsibility with ensuring quality of the product. For me, I rest easier at night using yarn in it's intended manner. If I'm reading an instruction manual and it says, "Please only use the authorized batteries" then I do not put batteries off of ebay into the device. Some are comfortable with that-- I am not.

I don't see how a little warning justifies banning the most popular (and bundled) package manager.
This doesn't apply if there's a safety (in the sense of dependency locking) benefit of course, but as far as I know there is none.

Benefits of yarn:

  • yarn is much faster than npm
  • yarn has workspace support
  • yarn has always had a much more reliable lock file. I have been monitoring this even since the days of npm's "shrinkwrap" command back in 2013. Even with npm fighting back with package-lock.json, yarn has had more deterministic results

@HoldYourWaffle
Copy link
Contributor

That makes a lot of sense, and with that additional information in mind I agree that it would at least make sense to "ban" npm. I'd say it's little "over-safe", but that's not really my place to talk about. Thanks for clarifying your viewpoint.

As for your original request for me to try it out: seems to work fine, although you should probably add some kind of message if it fails because package-lock.json exists. It saying "you should use yarn" when I was in fact using yarn confused me a little.

There's also this weird thing where it shows the echo command and then the output, not sure if that's fixable but it's a little annoying (nothing important, just a little nitpick).
Double echo output

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

Successfully merging this pull request may close these issues.

3 participants