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

feat: updated jest to v29 #415

Merged
merged 10 commits into from
Mar 20, 2024
Merged

feat: updated jest to v29 #415

merged 10 commits into from
Mar 20, 2024

Conversation

BilalQamar95
Copy link
Contributor

@BilalQamar95 BilalQamar95 commented Jul 12, 2023

Description

Breaking Change:

Updating jest from v26 to v29 introduces breaking change (except dropping Node versions), and that is to default snapshotFormat to {escapeString: false, printBasicPrototype: false}. This makes snapshots both more readable and more copy-pasteable.

@BilalQamar95 BilalQamar95 marked this pull request as ready for review August 17, 2023 10:05
@BilalQamar95 BilalQamar95 marked this pull request as draft November 17, 2023 11:44
@BilalQamar95 BilalQamar95 marked this pull request as ready for review February 23, 2024 10:40
@brian-smith-tcril
Copy link
Contributor

Has this been tested against any MFEs? If so, could you link draft PRs to those MFEs with this branch (or a branch created based on this branch with dist checked in) installed via git instead of npm?

Having a couple MFEs verified working (and knowing what issues arose in the process of making them work) will go a long way towards creating "how to upgrade your MFE to use jest v29" documentation.

@arbrandes
Copy link
Contributor

As per an internal conversation with @brian-smith-tcril, one way we could handle major upgrades to base libraries like this one is to start issuing bugfixes to the previous version, so there's (somewhat) less urgency in upgrading all downstream users.

Apparently, semantic-release provides for this rather neatly: https://semantic-release.gitbook.io/semantic-release/recipes/release-workflow/maintenance-releases#releasing-a-bug-fix-for-version-1.0.x-users. We'd just need to check that this and other similar repos like paragon and frontend-platform are configured accordingly. And, of course, to check with the corresponding maintainers if they're up to always backporting fixes.

@arbrandes
Copy link
Contributor

Oh, and if we know there are further breaking changes coming down the pipes, we should hold off and merge them all from the same branch, so only one major version ends up being bumped.

@arbrandes
Copy link
Contributor

(On the latter comment, this repo is already configured with an alpha pre-release branch, so we can use that.)

@BilalQamar95
Copy link
Contributor Author

@brian-smith-tcril Yes, this has been tested against several MFEs with drafted PRs in all of them. Here are 2 of them which i recently updated, frontend-app-account#958 & frontend-app-profile#932. I can update and link more if required

@brian-smith-tcril
Copy link
Contributor

@BilalQamar95 the CI is failing on both of the PRs you linked.

@BilalQamar95
Copy link
Contributor Author

BilalQamar95 commented Mar 11, 2024

@BilalQamar95 the CI is failing on both of the PRs you linked.

@brian-smith-tcril That would be due to frontend-build dependency since I'm installing it from a branch, these were tested out locally, all tests are passing for both PRs. I have since added overrides for both frontend-app-account#958 & frontend-app-profile#932 to clear out the CI, you can have a look at it again. The dependency issue for the consumers will be resolved once this PR is merged and respective drafted PRs are updated. I can update and link more PRs similarly, if required

@brian-smith-tcril
Copy link
Contributor

@BilalQamar95 looks good! Would you mind also linking PRs showing CI passing on frontend-platform, frontend-component-footer, and frontend-component-header?

@BilalQamar95
Copy link
Contributor Author

@brian-smith-tcril Sure, I have updated PRs for requested MFEs: frontend-platform#661, frontend-component-header#369 & frontend-component-footer#317.

@BilalQamar95
Copy link
Contributor Author

@brian-smith-tcril please let me know if is there anything else I can help with in order to get this reviewed & merged

@arbrandes
Copy link
Contributor

I have a question as well: should we bundle this into the alpha branch first, and release everything in one go?

@jristau1984
Copy link

@arbrandes is there any reason to take that approach? The goal of this PR is to move the Jest upgrade forward, and bringing in other alpha-release related changes has a chance to create unnecessary noise to achieving that goal.

I am fully supportive of getting the alpha branch into main, but I think they should be considered distinct efforts.

@jristau1984
Copy link

@arbrandes is there any reason to take that approach? The goal of this PR is to move the Jest upgrade forward, and bringing in other alpha-release related changes has a chance to create unnecessary noise to achieving that goal.

I am fully supportive of getting the alpha branch into main, but I think they should be considered distinct efforts.

I believe this was followed up on in the Frontend WG, and now has a path forward.

Muhammad Abdullah Waheed Khan
in the frontend working group, its decided to test alpha with MFEs that do not typescript, and if it doesn't break anything other than some basic config changes, we'll merge jest with alpha and release these both simultaneously as single breaking change

@BilalQamar95 BilalQamar95 changed the base branch from master to alpha March 19, 2024 11:35
@abdullahwaheed
Copy link
Contributor

@arbrandes @brian-smith-tcril this PR is rebased with alpha and now ready to review and merge

@arbrandes
Copy link
Contributor

@jristau1984,

The goal of this PR is to move the Jest upgrade forward, and bringing in other alpha-release related changes has a chance to create unnecessary noise to achieving that goal.

That's a good point, and we may still end up splitting the Typescript work from Jest 29, but there are a couple of other considerations:

  • Both the Typescript (currently in alpha) and Jest 29 efforts result in breaking changes. Instead of having one breaking change followed by another, I suspect it will be more efficient to handle both at once across the board.
  • As I understand it, one of the blockers to getting Typescript into master is precisely Jest 29.
  • The Typescript work has been parked in alpha for quite a while, now. It's high time it either got merged to master or dropped. If we can borrow some momentum from the Jest 29 upgrade to make a decision, so much the better.
  • The alpha branch is meant to be used as a way to test breaking changes downstream before we release them (something which will come in handy for Jest 29, I'm sure), and it just so happens that Typescript is already in there.

In any case, we should revisit this strategy if we run into any blockers.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Any objections to merging this into alpha, @brian-smith-tcril?

@arbrandes
Copy link
Contributor

Once it's in alpha, we should pick a couple of MFEs to upgrade to the alpha package (probably the ones that were already tested with Jest 29), and if it looks good, we go ahead with merging alpha to master here.

@arbrandes
Copy link
Contributor

We should probably also have the alpha branches in frontend-platform, -header, and -footer churning out packages. Let me check if they're set up properly.

@abdullahwaheed
Copy link
Contributor

We should probably also have the alpha branches in frontend-platform, -header, and -footer churning out packages. Let me check if they're set up properly.

We have tested account, profile and learning MFE with alpha and jest with test branches and they were working fine

@arbrandes
Copy link
Contributor

arbrandes commented Mar 20, 2024

Ok, see the PRs I just mentioned. Once merged, we can make alpha packages that depend on the frontend-build alpha, and finally port at least one MFE (better a couple) to actually use them in ther respective master branches. That should tell us right quick if there are any issues. :)

@arbrandes
Copy link
Contributor

Alright, let's get this into alpha.

@arbrandes arbrandes merged commit c6372b7 into alpha Mar 20, 2024
5 checks passed
@arbrandes arbrandes deleted the bilalqamar95/jest-v29-upgrade branch March 20, 2024 16:34
@openedx-semantic-release-bot

🎉 This PR is included in version 14.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

arbrandes pushed a commit that referenced this pull request Apr 19, 2024
This introduces two main features:

* feat: Re-enable typescript for production builds (introduced directly in this `alpha` branch)
* feat!: updated jest to v29 (introduced originally via #415)

It also includes a series of minor workflow and dependency updates.

BREAKING CHANGE: Updating jest from v26 to v29 changes the default snapshotFormat.
@openedx-semantic-release-bot

🎉 This PR is included in version 15.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

6 participants