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

Re-add the PRs that are reverted from main due to introducing breaking changes to github.com on the v35.26.0 release #3429

Closed
broccolinisoup opened this issue Jun 20, 2023 · 3 comments
Assignees
Labels

Comments

@broccolinisoup
Copy link
Member

Due to the recent deploy freeze at dotcom, we were unable to release primer/react on a regular schedule and this impacted the scope of the release candidate, therefore made it very challenging for us to address failing tests at dotcom. As a team, we made a decision to revert the PRs that we are introducing breaking changes to be able to get the current release out. Here is the PR that reverts the PRs that introducing breaking changes.

This issue focuses on adding them back to main to be released in one of the upcoming releases.

If you are assigned to this issue, it is high likely that we had to revert your PR(s) (apologise again ❤️ ) but now it is time to add back in.

Create a (draft) PR in primer/react

Create a new (draft) PR in https://github.com/primer/react with your reverted changes. You can cherry-pick the reverted commit on top of main or if you have a different idea/way of doing it, feel free to do it. If you decide to go with cherry-pick, you can find the commit ID to cherry-pick in your original PR, towards the bottom. Example: "Merged via the queue into main with commit 9532977 on May 16" Sorry if you already know about this stuff, I just wanted to make sure we don't block anyone in case they are not familiar with reverting/re-adding commits.

For example, I created one for my reverted octicon changes #3428

Canary version of @primer/react at PR time

We release a canary version of primer react, everytime when there is a new PR created as well as the PR receives new commits. If you scroll to the CI checks of your newly created draft PR, you will see one job that publishes these canary packages. ie.e "Published @primer/react — 0.0.0-xyz" - You can use this 0.0.0-xyz version to test your changes at github.com

Dotcom development environment setup

Open up your dotcom development environment. If this is your first time, please refer to the hub documentation. If you have any problems setting up your dotcom environment, please ask help on these slack channel.

Installing the canary version at dotcom

Now we can install the canary version 0.0.0.xyz at dotcom (hub reference)

bin/npm i @primer/[email protected]  -w npm-workspaces/primer

Above comment will do most the work but we need some manual work to clean up. Please see this discussion (very short one, don't worry!) if you are curious about why node_modules might get messed up. Feel free to follow the instructions on the discussion or if you are already aware of this problem and have your own way of resolving, feel free to do it. I am just going to list clean up steps that I have found working nicely.

  • open package-lock.json
  • remove keys node_modules/@primer/react* (not the react-brand ones)
  • remove keys npm-workspaces/primer/node_modules/@primer/react*
  • remove the node_modules folder from npm-workspaces/primer
  • bin/npm i

The main idea of this clean up is that we don't want any node_modules under npm-workspaces/primer

Testing changes at dotcom

Hopefully things go smoothly on the previous step and you can now test your changes. We usually push a draft PR to dotcom and rely on CI checks to see if the changes has any issues with integrating in dotcom. Feel free to run tests/checks on your CodeSpace if this is your prefered way of testing the changes.

After identifying the failings/issues with your PR, there are possibly 2 ways to go forward:

  1. Implement backwards compatible fixes/enhancements
    If you feel comfortable implementing backwards compatible fixes/enhancements, great! If not, no worries. Please tag us @primer/react-reviewers on your PR and we can look into the PR and can suggest backwards compatible alternatives.
  2. Fixing the issues at dotcom
    If the PR cannot be introduced with a backwards compatible implementation, we should fix the breakings/failings at dotcom.

We are aiming to see all green checks on our integration PRs at dotcom so that we can merge our primer/react PRs confidently.

I hope this is helpful, sorry it is a bit long! Please let me know if you have any concerns/questions/feedback or recommendation of the workflow!

@broccolinisoup
Copy link
Member Author

@adrianababakanian I can't add you as an assignee, probably because you are not in primer org. Feel free to create your own tracking issue, or use this to refer the work you are doing to re-introduce the changes. However works for you!

@radglob I'll reach out to you to pair on the ActionList stuff!

@broccolinisoup
Copy link
Member Author

broccolinisoup commented Jun 20, 2023

Also, we still haven't upgraded the @primer/react version at dotcom to v35.26.0 https://github.com/github/github/pull/276739/ We are doing review lab tests. It would be easier to start the work of re-introducing the changes once we finalise the upgrade. I'll comment here once we complete the upgrade ✨

@broccolinisoup
Copy link
Member Author

Hey folks 👋🏻 Just to let you know that we merged the integration PR and dotcom now has @primer/react v35.26.0 🎉

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

No branches or pull requests

5 participants