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 frontend-build to bump jest version to v29 #366

Merged
merged 6 commits into from
Apr 24, 2024

Conversation

BilalQamar95
Copy link
Contributor

@BilalQamar95 BilalQamar95 commented Dec 12, 2023

Description

  • Updated frontend-build to v14
  • Updated frontend-platform to v8

@BilalQamar95 BilalQamar95 marked this pull request as ready for review April 22, 2024 14:30
@BilalQamar95 BilalQamar95 changed the title chore: bumped jest to v29 feat!: Updated frontend-build to bump jest version to v29 Apr 22, 2024
@BilalQamar95 BilalQamar95 self-assigned this Apr 22, 2024
package.json Outdated
@@ -61,7 +61,7 @@
"lodash": "^4.17.21"
},
"peerDependencies": {
"@edx/frontend-platform": "^7.0.0",
"@edx/frontend-platform": "^8.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"@edx/frontend-platform": "^8.0.0",
"@edx/frontend-platform": "^7.0.0 || ^8.0.0",

i think we should add support of frontend-platform v7 here as well because a consumer will not break if it uses v7 or v8 of frontend-platform since frontend-platform v8 doesn't have any breaking changes except frontend-build peer dependency change. Moreover, we are opening PRs in respective MFEs as well with frontend-build and frontend-platform upgrade, i think we can handle this manually in those PRs. In this way we don't have to release it as a breaking change.

Please do this same change for all the respective PRs. We should not comment same thing on each MFE

Copy link
Contributor Author

@BilalQamar95 BilalQamar95 Apr 23, 2024

Choose a reason for hiding this comment

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

We should not comment same thing on each MFE

I'm curious could you clarify the specific concerns with documenting this change directly within each relevant PR, especially when the note is very brief yet critical in dictating how the change should be considered a minor, not a major version?

It is my understanding that documenting changes individually in each PR enhances clarity and accessibility for anyone reviewing the changes now or in the future. It eliminates the need for additional navigation and cross-referencing, which can be prone to oversight and errors. Moreover, including all pertinent information within the PR itself upholds a standard of transparency and self-sufficiency, fostering a more robust and understandable codebase.

More importantly I believe review process in my opinion is designed to ensure clarity and thoroughness, ensuring that any member of the team or future developers can understand the rationale behind changes without having to refer back to other discussions or PRs. Ensuring each PR is self-explanatory is not just good practice imo, especially when its easily manageable as including a brief explanatory note in few PRs. What are the advantages you see in not doing so and compromising these standards that might outweigh the aforementioned benefits? I'm curious to know since this is a common scenario and individual documentation for different PRs was encouraged in every other case that I have seen.

Copy link
Contributor

Choose a reason for hiding this comment

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

since we are making same changes across different MFEs, we should not repeat same things in different PRs. We have been doing the same on other places as well, i.e. get comment on one PR and made such changes across the board [ref]

Copy link
Contributor Author

@BilalQamar95 BilalQamar95 Apr 23, 2024

Choose a reason for hiding this comment

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

In case you have missed it in your scenario with browserslist, it is managed under a centralized epic, which involves a series of interconnected tasks designed to maintain consistency across multiple repositories, holding all the details which can be easily referenced to. Also it goes across all MFEs while we are dealing with very few in comparison, and that to without a centralized epic holding all the details which can be referenced to and involved note is very brief yet critical in dictating how the change should be considered a minor, not a major version. This discussion is less about this specific issue about more about general approach to such scenarios thats why I was curios regarding your rationale to it as opposed to the benefits that come by simply adding brief notes on the few PRs.

i.e. get comment on one PR and made such changes across the board [https://github.com/openedx/paragon/pull/1664#discussion_r999875109]

My point is of it being about enhancing clarity and accessibility for anyone reviewing the changes now or in the future, in this case there is a centralized epic to achieve that, also in our case there are very few MFEs with no centralized document/epic detailing the decision making unlike your example.

Again I believe review process in my opinion is designed to ensure clarity and thoroughness, ensuring that any member of the team or future developers can understand the rationale behind changes without having to refer back to other discussions or PRs especially since MFE could fall under different owners and in our case these is no centralized document for it either .

@BilalQamar95 BilalQamar95 merged commit 226e90d into master Apr 24, 2024
5 checks passed
@BilalQamar95 BilalQamar95 deleted the bilalqamar95/jest-v29-upgrade branch April 24, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants