-
Notifications
You must be signed in to change notification settings - Fork 43
docs: Add Frontend Dependency Freeze ADR #753
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
base: master
Are you sure you want to change the base?
Conversation
2cafddf to
4f75dd1
Compare
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependencies.rst
Outdated
Show resolved
Hide resolved
|
Could we do this in a simpler way, by saying that the versions of these major dependencies should be specified by |
|
@bradenmacdonald, somewhat surprisingly, in the beginning of the development of frontend-base we ran into problems tyring to do just that. I forget what the hitch was (it could have been due to module federation, or maybe just Typescript not working properly after a local That said, this ADR is more about the communication and less about the mechanics. Sure, we can try to code the consequences as simply as we can (including what you suggested), but we still need to decide - as early as we can - what version of React we're going to use for Verawood, Wyllow, and so on, and then to communicate that out to maintainers/developers. |
4f75dd1 to
6803bc4
Compare
|
@arbrandes how can we get this merged? |
|
I haven't seen any more objections, so I guess we're good to change it to Accepted and then to merge it! |
And do it under OEP-0010, since it is mostly about releases and release schedules.
4e749a0 to
5aae89c
Compare
|
@sarina, mind giving it a thumbs up? |
|
I think I'd prefer for someone on the FWG to give final approval :) |
|
@feanil this was discussed in the last maintenance WG, we'd appreciate if you could take a look! |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great! I left a few comments with thoughts/questions. Looking forward to hearing back!
oeps/processes/oep-0010/decisions/0001-frontend-dependency-freeze.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependency-freeze.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependency-freeze.rst
Outdated
Show resolved
Hide resolved
|
|
||
| Once the Dependency Freeze is announced, frontend maintainers must abide by it for the duration of a given release cycle. Speficically, this means that on all main branches, corresponding dependencies should not be bumped beyond the defined versions from the moment of the announcement until immediately after the cutoff. And on release branches, the same applies for the life of the branch. This constraint will not only give developers, testers, and ultimately, users, a firmer base to work off from, but also a more consistent one both visually and under the hood. | ||
|
|
||
| Patch version updates are not foreseen to be problematic, as in a frontend-base paradigm the dependency versions this ADR concerns itself with will be configured with a ``^`` (caret) prefix as ``peerDependencies`` in all relevant ``package.json`` files. In practice, this means that whenever a new non-major (i.e., non-breaking) version of a library is published, it ends up being used automatically by all apps at build time as a result of NPM's deduplication algorithm. In other words, there's no need to issue a PR to every single ``package.json`` just because a bug was fixed in Paragon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch version updates
configured with a
^(caret) prefix
^ allows for greater minor/feature versions as well.
I think a bit of clarity around how the freeze works would be helpful.
MFE apps:
- Use "compatible" (
^) inpeerDependenciesinpackage.json, this enforces- Maximum AND minimum major/breaking version (these are the same number) (
^6or just6are valid and equivalent inpackage.json) - (Optionally) minimum minor/feature version (
^6.3is valid inpackage.json) - (Optionally) minimum patch/fix version (
^6.3.1is valid inpackage.json)
- Maximum AND minimum major/breaking version (these are the same number) (
frontend-base itself (I'm not sure here), are we:
- Using explicit (
6.3.1) versions? Are we manually bumping patch versions forfrontend-baseitself? - Using "approximate" (
~6.3.1) versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I agree here, more documentation on what exactly you want folks to pin to with an example would be helpful for connecting the abstract goal to concrete actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I added explicit instructions to the Decision section.
As for major vs minor vs patch in peerDeps, I think we should stick to ^<major>. It's both expressive and simple. And, of course, it does what we need it to do.
Regarding frontend-base, I see no reason not to do the same thing in its package.json, at least as far as this ADR is concerned. Sure, there will situations where we want to be more specific (buggy patch versions come to mind), but as long as we don't explicitly forbid being more specific here, I think in the ADR it should be lumped in with everything else.
(If we ever want a more nuanced version control strategy, it probably deserves its own ADR in frontend-base itself. This one's more about release coordination around major versions.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new wording in 6da649e is very clear! LGTM!
|
|
||
| Otherwise, we reject a smaller set of frozen dependencies. The proposed list is considered to be the smallest one that still achieves the goals of consistency, maintainability, UX, and DX. Notably, not freezing Paragon can lead to significant UX consistency issues, and the freezing of Frontend Base is a fundamental technical requirement for all apps built on it - not to mention the benefits we get from de-duplication. The latter, in turn, necessitates the freezing of the other dependencies such as React itself, which doesn't support having multiple versions running on the same web page. | ||
|
|
||
| We also reject a larger, more prescriptive set of dependencies. While more may eventually be added to the list, developers are, in principle, free to add other requirements to individual package.json files, even if different apps end up using different versions of the same dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably out of scope for this ADR, but we should probably start thinking about a process for "auditing" MFE app dependencies to see if we should move them up frontend-base/to peer in the apps.
I assume for the first few releases it'll be a non-issue as ^ versioning of the same dependencies used in multiple MFE apps will "just work" to dedupe, but as time goes on and some MFE apps are more actively maintained than others that could change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed: both that it's out of scope here, and that we should start thinking about it. :)
I imagine the first time this comes up organically it'll be when some older app uses, say (since it actually happened recently) react-query v4, while another users react-query v5, and something breaks when they're built together via frontend-base, before this ADR added react-query to the list.
I feel like we can just cross that bridge when we get there. In all likelihood, somebody will report a bug, and to fix it we'll just add the dep to the list. 🤷🏼♂️
|
@arbrandes ping - could you please reply to review comments? @feanil ping - could you take a review pass? |
feanil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments but the overall approach makes sense to me. I want to push us to make tickets where possible for this and also agree with @brian-smith-tcril that tooling to detect repos that are out of compliance will be important. The repo-health-checks tooling might be useful here and worth looking into as follow-up work.
oeps/processes/oep-0010/decisions/0001-frontend-dependency-freeze.rst
Outdated
Show resolved
Hide resolved
oeps/processes/oep-0010/decisions/0001-frontend-dependency-freeze.rst
Outdated
Show resolved
Hide resolved
|
|
||
| (This list may be modified in the future to reflect changes in the technology stack, such as those stemming from changes to :ref:`OEP-67 <OEP-67 Standard Tools and Technologies>` or the introduction or removal of an Open edX library.) | ||
|
|
||
| The decision will be posted in the appropriate public channels, so that by the time of the cutoff, all aforementioned repositories' main branches must be using the corresponding versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about creating tickets on all repos that need to be in sync, then maintainers just need to track issues on their repo which is something we want them to be doing anyway. It also gives us good visibility into completion status with a parent ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Added as a specific type of public channel.
|
|
||
| Once the Dependency Freeze is announced, frontend maintainers must abide by it for the duration of a given release cycle. Speficically, this means that on all main branches, corresponding dependencies should not be bumped beyond the defined versions from the moment of the announcement until immediately after the cutoff. And on release branches, the same applies for the life of the branch. This constraint will not only give developers, testers, and ultimately, users, a firmer base to work off from, but also a more consistent one both visually and under the hood. | ||
|
|
||
| Patch version updates are not foreseen to be problematic, as in a frontend-base paradigm the dependency versions this ADR concerns itself with will be configured with a ``^`` (caret) prefix as ``peerDependencies`` in all relevant ``package.json`` files. In practice, this means that whenever a new non-major (i.e., non-breaking) version of a library is published, it ends up being used automatically by all apps at build time as a result of NPM's deduplication algorithm. In other words, there's no need to issue a PR to every single ``package.json`` just because a bug was fixed in Paragon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I agree here, more documentation on what exactly you want folks to pin to with an example would be helpful for connecting the abstract goal to concrete actions.
|
I've addressed the latest comments. |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
|
@feanil, want to give it a last browse? Can we merge it? |
Do it under OEP-0010, since it is mostly about releases and release schedules.