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

WIP + RFC - chore(docs): Add node style guide supplemental #517

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

StaberindeZA
Copy link
Contributor

Because

  • Add supplemental section to node style guide to provide additional information

This pull request

  • Adds supplemental section with Software Architecture Implementation doc

Because:
* Add supplemental section to node style guide to provide additional
  information

This commit:
* Adds supplemental section with Software Architecture Implementation
  doc
@StaberindeZA StaberindeZA changed the title chore(docs): Add node style guide supplemental WIP + RFC - chore(docs): Add node style guide supplemental Apr 19, 2024
@StaberindeZA StaberindeZA marked this pull request as draft April 19, 2024 16:23
Copy link
Contributor

@LZoog LZoog left a comment

Choose a reason for hiding this comment

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

Just a couple drive-by nits.

'reference/style-guides/supplemental/software-architecture-implementation',
],
},
'reference/style-guides/react-style-guide',
]
},
{
Copy link
Contributor

@LZoog LZoog Apr 22, 2024

Choose a reason for hiding this comment

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

Is there any way to include the graph in whatever raw format/non-image as well so it's more easily editable later if needed? Not sure what tools we are all using for charts.

@@ -0,0 +1,155 @@
---
title: Software Architecture Implementation
Copy link
Contributor

@LZoog LZoog Apr 22, 2024

Choose a reason for hiding this comment

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

Edit: probably never mind. I was looking at this from the direct link and totally thought I was in adrs/ in fxa.

Should this be more specific? If I'm looking at the title list in adrs/ and I see this, I'd have to click into it to understand what this was covering specifically.

Comment on lines +51 to +53
* Client => StripeClient ([code](https://github.com/mozilla/fxa/blob/main/libs/payments/stripe/src/lib/stripe.client.ts))
* Manager => StripeManager ([code](https://github.com/mozilla/fxa/blob/main/libs/payments/stripe/src/lib/stripe.manager.ts))
* Service => StripeService ([code](https://github.com/mozilla/fxa/blob/main/libs/payments/stripe/src/lib/stripe.service.ts))
Copy link
Member

@bbangert bbangert May 2, 2024

Choose a reason for hiding this comment

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

This is probably a bad example, because this demonstrates organization by the specific 3rd party implementation rather than organizing by feature. In organizing by feature (as shown the repository example blow), one would instead off a StripeClient, an InvoiceManager/SubscriptionManager/CustomerManager/etc, and then an appropriate service above based on the feature in use.

The problem with this example is that the StripeManager ends up duplicating the StripeClient for the most part, rather than implementing invoice or customer specific logic that may involve multiple clients (e.g. asking the InvoiceManager to pay an invoice would involve the PayPalClient and StripeClient, and even several repository methods). It also means that StripeManager becomes a massive class like our current StripeHelper, rather than organizing by feature which keeps them smaller.

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

Successfully merging this pull request may close these issues.

None yet

3 participants