-
Notifications
You must be signed in to change notification settings - Fork 34
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
docs: OEP-67 - Document our best practice tools and tech. #518
Conversation
This is great! It looks like everything (except for Codecov) from OEP-11 can go under the "Frontend Technology Selection" section as it's all pretty frontend specific. |
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.
Review of the current draft.
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.
Thanks Feanil. This is long overdue. A few comments but I'm very supportive of the overall direction.
* OEP-11 will be superseded. | ||
|
||
* All ADRs under OEP-11 will be moved to be under this OEP instead. |
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.
Yay for consolidation!
#. **Use Redux** | ||
|
||
For state management of complex | ||
client-side interactions, Redux must be used. This library was chosen | ||
because it sees strong use in the React community, but is also flexible | ||
enough to be used in situations where a hybrid React/Backbone architecture | ||
exists. |
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.
I remember David Joy saying at one point that Redux wasn't worth the hassle and that MFE devs should feel free to simply not use it.
I'm not sure if I remember that right or whether that statement still applies, I'd need @arbrandes or @brian-smith-tcril 's input.
(I guess this is the sort of thing that will make this OEP useful :)
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.
This is a copy from the current frontend OEP. I think any corrections should be made as follow-up ADRs and updates to the OEP.
Python code - see `OEP 18`_ for more information. For more information on | ||
package-lock files see `Package Lock`_. | ||
|
||
#. **Keep dependencies up to date by using Greenkeeper** |
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.
Shouldn't this recommend Renovate instead? See https://openedx.atlassian.net/wiki/spaces/AC/pages/1841791377/Set+up+Renovate+to+Automate+JavaScript+Dependency+Updates#Notes-on-Alternatives for reasons we started to ditch Greenkeeper. As with the Redux note above we could update it after initial release, but I'd rather not risk people trying to start switching to Greenkeeper after reading this.
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 think this should be a follow-up task, I made #524 to track the work.
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.
all but one of my comments are grammar nits, for which I apologize; , because I know it is early days yet. It's just, it seemed a waste not to make the notes as I read.
|
||
#. **Use React** | ||
|
||
**Rationale**: React must be used for building new UIs, as it is |
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.
re: must. I am new enough around here that I don't know the standard, but I always assume that if a spec is using RFC 2119 verbs meaningfully, they should be capitalized, e.g. MUST, MAY, just to clarify what requirements really are.
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.
that's a fair point, I'm not sure what the intent of the original authors was here, per your second point, MUST feels like a lot here to me as well but I'm not sure what the current stance should be. This is probably worth a follow-up conversation in the frontend working group. @brian-smith-tcril @arbrandes @adamstankiewicz what do you think?
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.
Yeah, I actually think we're squarely in SHOULD
-land; even React. This definitely deserves a dedicated conversation.
|
||
#. **Use React** | ||
|
||
**Rationale**: React must be used for building new UIs, as it is |
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.
Is the guidance really MUST on React? Some UIs are going to be so simple that react is overkill. (See above regarding: "new around here"; if this debate has already been had and resolved then I don't need anybody to restart it for me. It's more a question about unexamined assumptions.)
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.
@deborahgu If a UI isn't using React, at least one possible concern is the compatibility with using the Paragon design system and React-based UI component library in terms of brand consistency, etc. across UI projects within Open edX.
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.
yeah, this is a bigger question than can be resolved in this OEP, but eg. right now we're working on a place where we're dealing with the entire Paragon dependency stack to render 100% static pages, and it's not lightweight. It might be fundamentally worth the cost, but the cost is not small.
@bradenmacdonald the review period is complete, I think there are a lot of follow-up updates but as far as I can tell, no disagreement on this OEP existing and the goals it's trying to solve for. Do you have any concerns that I should address because I make the rest of the re-organization changes and land this OEP? |
@feanil Nope! I don't have any concerns, and I think people seem quite happy with this change. (And eager to make the follow-up updates, as it seems like quite a few things have gotten outdated from the original.) |
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.
LGTM, pending a couple formatting fixes.
Consequences | ||
************ | ||
|
||
* OEP-11 will be superseded. |
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.
Friendly reminder to go into OEP-11 and make this change.
@bradenmacdonald good call, I'll make a new top-level issue and collect all the known updates we need into that for now. |
acd775a
to
5c86934
Compare
@bradenmacdonald I've created #531 to track the current known updates that we need. Let me know if you think there's anything else that needs to be done before we land this. |
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.
We currently have some of this for the frontend but having something a bit more standard acrooss the entire ecosystem would be helpful for making decisions going forward. We also: * Replace OEP-11 and relocate its ADRS to OEP-67 * Add redirects from old doc locations to the new locations for any docs that moved. Co-authored-by: Kyle McCormick <kdmc@pm.me> Co-authored-by: Ned Batchelder <ned@edx.org> Co-authored-by: Deborah Kaplan <deborahgu@users.noreply.github.com> Co-authored-by: Braden MacDonald <braden@opencraft.com>
3689a0d
to
80aeb92
Compare
The topic for consideration in this PR
We should have an OEP to help us know which tools and technology we as a community should standardize on. This OEP will help us track these items and provide a structure place for us to have future conversations when we want to change community standards.
We currently have some of this for the frontend but having something a
bit more standard across the entire ecosystem would be helpful for
making decisions going forward.
Note for Reviewers: The tools/standards in the frontend section were simply moved from OEP-11, they are NOT up for re-consideration in this PR. If they are wrong or need updates, I recommend supporting this OEP and then making a new ADRs to update a recommendation that you feel is incorrect.
Tasks for After the OEP is Approved