Patternfly-react: Hard to work with the repo as yarn.lock is present

[skateman]

Hi,

due to the yarn.lock in the master branch my intern got into an endless loop of rebasing his PR every second day. This is really frustrating and IMO this could be solved by just not having the lockfile in that branch. I understand that the file is needed in production, but would it be possible to have it in the releases and version tags only?

Thanks

cc @Hyperkid123 @karelhala

[himdel]

:+1: Lockfiles really have no bussiness living on the development branch.

If needed, they can be created as part of the npm build process.

[skateman]

@rachael-phillips could you please do something about this, it is blocking a lot of our devs to contribute to PF.

[Hyperkid123]

Yep lock files should be part of production build. If there is some danger of breaking changes after updating dependencies you should lock the version in package.json in dev environment.

[AllenBW]

Oh gosh, merging lock files is madness. Best to just accept master changes, throw it away (the lockfile), then rebuild it. Though the idea of only having it in a build branch is intriguing. 🤔

[Hyperkid123]

@AllenBW well it should to be in production build to make sure the package will have correct version of its dependencies that you know for sure they work (tests, build etc.). Otherwise may happen that somewhere in consumers app it will install newer version of some PF with breaking changes of its dependency and everything is busted.

[rachael-phillips]

Hi @skateman , I see this. I have a meeting this morning where I will discuss with our team. I will get back with you soon.

[AllenBW]

Yeah! Agreed absolutely. Must have for production, I was more so intrigued by only having it in production @Hyperkid123 In https://github.com/RedHatInsights/insights-advisor-frontend we lock our deps to exact versions so we there is no question as to what is being build/used.

[Hyperkid123]

@AllenBW

Yeah! Agreed absolutely. Must have for production, I was more so intrigued by only having it in production @Hyperkid123 In https://github.com/RedHatInsights/insights-advisor-frontend we lock our deps to exact versions so we there is no question as to what is being build/used.

Yes this is something that i personally like to handle in package.json directly by locking specific version of certain packages that we know break from time to time. I don't like PRs with 2k line changes where 99% comes from lockfiles.

[dgutride]

Why is the user's lock file changing regularly? Running yarn install after a pull just updates the node_modules, it shouldn't overwrite the yarn file.

[himdel]

@dgutride In the past 3 weeks, yarn.lock was rewritten 10 times on master. (ref)

So.. any PR opened in last 3 weeks that's otherwise ready and waiting for review would have to have been rebased 10 times during that time.

You can see some frustration about this, for example in https://github.com/patternfly/patternfly-react/pull/864.

It makes it practically impossible to contribute here..

[Hyperkid123]

@dgutride In the past 3 weeks, yarn.lock was rewritten 10 times on master. (ref)

This changes can be caused by different version of yarn/npm on your system. I've encountered this issue before in one MIQ repos.

[redallen]

The lockfile is necessary to lock not only dependencies in our package.json files, but also to lock the 2nd level dependencies _of those dependencies._ Sane builds are difficult without it. It's not going away.

If you are upset using git rebase to merge master's lockfile changes _one commit at a time_, here's a workflow for you that doesn't use git rebase.

1. git merge upstream/master
2. git rm yarn.lock
3. yarn install
4. git add yarn.lock
5. git commit -m "Merge upstream/master" --no-verify
6. git push

The price to pay over a rebase is one more commit in the history of your PR, which is hardly anything. As an example, I just did this on #864 .

[himdel]

@redallen Merge != rebase.

It's bad form to merge pull requests with merge commits in them, so... this does not really solve the problem.

And yes, it is not that hard to rebase even with yarn.lock. The problem is not doing the rebase. The problem is having to rebase in the first place.
The problem is having one single file that everybody keeps changing and therefore breaking everybody else. It's the same problem as having a "please update the changelog in every PR" rule.

I do understand that locking dependencies of dependencies is an otherwise unsolved problem, but... is the breakage from those really that severe to warrant making things annoying for everybody else?

Because, me, I'm pretty sure at this point I wouldn't be likely to contribute to this repo after seeing all this. (And the review problems over the years.) Maybe I'm a snowflake, maybe you're losing contributors. :)

EDIT: since commit yarn (1.0+) can deal with merge conflicts, so the rebase should be as simple as yarn ; git add yarn.lock ; git rebase --continue.

---

But to be a bit constructive here, have you tried addressing the dependencies of dependencies problem?

Usually when that happens, it's because somebody is claiming to have dependencies when they really should have peerDependencies.
And once those are fixed as peerDependencies, you can usually fix the version in package.json.

[himdel]

But maybe there is a technical solution on this side too:

if we .gitignore yarn.lock but keep it in the repo, it won't get randomly updated by contributors, so there will be no conflicts,
all we need to do after that is have a bot run yarn ; git commit yarn.lock -m whatever; git push whenever a PR touching package.json gets merged, so the lockfile will be up to date

(And the "bot" bit can probably be done on travis.)

An additional advantage is that you can have daily auto-updates with tests, only merging the change if passing. And backups of yarn.lock from each day.

EDIT: also, did I mention no more conflicts? :)

[skateman]

I'm not really convinced by the bot :confused: it just pushes the collisions one level lower, to the client. But it's truly better than nothing.

[himdel]

I'm hoping the client would never see collisions if the file is gitignored.

But it's truly better than nothing.

True true :)

[martinpovolny]

Because, me, I'm pretty sure at this point I wouldn't be likely to contribute to this repo after seeing all this. > (And the review problems over the years.) Maybe I'm a snowflake, maybe you're losing contributors. :)

I can assure that you are not alone with this view.

[karelhala]

I don't have strong feelings against pushing yarn.lock to devel branches, but I can see that a lot of devs are using different version of yarn (including me). With version 1.10.0 of yarn it automatically updated integrity hashes even if I did no changes to dependencies.

I was unaware of this as well and pushed multiple times integrity hashes to my PRs as well.

But not to diverte from the original topic, as the repository is huge and uses a lot of dependencies for multiple packages I can understand the need of lock file. It would be impossible to track all of 3rd party deps and manage them in package.json file. If we look at how others do this babel and react are pushing their lock file as well. So if you are making any changes to any dep (adding new package, updating package) it really is necessary to push it lock file as well.

But the problem I am hearing from all these comments is slow review process. I particularly like the idea that each opened PR needs to have assignee to it assigned either on PR creation or when contributor is happy how the code looks like. This assignee should be responsible for fast review process from all parties (UX, code, accessibility, tests, etc.) and communicate directly with the developer if any changes (code review requests, merge conflicts) are required from his side or ping reviewers if new changes were applied to such PR.

I've been contributing to this repository for some time and developer experience went up at one point, but right now it's not improving at all. I feel that it is getting worse actually in recent weeks, so let's understand what is causing these frustration and fix them.

[redallen]

Closing due to inactivity. Would love discussion on PR processes in new issue if you're interested @karelhala .

[martinpovolny]

@redallen : There was no activity on this issue, because there was no response from the PF side. Do you expect people to keep posting in an issue w/o response to keep it open? Or should people copy the texts above to a new issue?