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

Added the conversation as a detailed explanation for create layers #394

Closed
wants to merge 1 commit into from
Closed

Conversation

bikingbadger
Copy link

@bikingbadger bikingbadger commented May 1, 2019

Using the conversation in #252 I updated the create layers section with more detail. I added the credits of those involved. Please let me know if anything should be updated or changed.

This will close #393

@js-kyle
Copy link
Contributor

js-kyle commented May 6, 2019

@bikingbadger thanks! great idea to capture the discussion that has happened over into the content.

@goldbergyoni
Copy link
Owner

@bikingbadger Thanks for this important contribution 🎆

I'm not sure whether the discussion there is exhaustive enough to summarize this topic, I'd like to work on it with you and improve a bit. For example, if we discuss layering we should mention the various techniques to achieve this (e.g. clean architecture, hexagonal, n-tier, DDD, onion, etc).

I'll share with you some additional insights we should cover in few days, cool?

/remind me in 4 days

@bikingbadger
Copy link
Author

@i0natan Great, glad to work together on this. I'm still very much learning so I was just looking to flesh out this section a bit more as it was quite bare. I saw the conversation and that is why I referenced it. I'd really love some further information on the subject as I'm starting on a side project and trying to use what I'm learning here in the actual project.

@bikingbadger
Copy link
Author

@i0natan you up for discussing this in a bit more depth so I can make the changes?

@goldbergyoni
Copy link
Owner

@bikingbadger Sorry for the delay, I'm fully up now for discussing this.

Are you available to run this discussion over the next week simultaneously? I've several ideas for improvement to share

Note to self: express->web framework, mock->test, mention few layering techniques, better diagram, mention the great movie about clean architecture, consider separating into 2 bullets: entry points vs the domain structure, code exampe to share, etc

@bikingbadger
Copy link
Author

What do you mean simultaneously? Using the comments here or getting on skype or something to knock this issue out.
I'm in Israel so not sure how your timezones work but sure we can work something out. I'm here to make any changes you reckon would be good for the PR

@goldbergyoni
Copy link
Owner

@bikingbadger Whatever works for you, I meant just exchanging here messages every day (once a day is 100% fine or whatever pace suits your timeline).

My thoughts: This bullet actually handle two different topics: (A) separate entry-points from the app - express, CLI, Electron, whatever app should be isolated from the logic/data and the app internal (B) separate the domain - the app logic should be isolated from all the technical stuff (DB, log, etc). This principle is a commonality of architecture types. Let's split?

About 'Separate entry point from the app'

  • Motivation: simply put, if you mix your access point (e.g. api objects) with the rest of the app, for example by passing {req, res} to many methods, then your app become coupled to one technology/access and once you want to consume it using a different technology (e.g. add graphql, desktop app, CLI, etc) - a big refactor is needed. Moreover, it's much harder to unit test internal modules as they depend on API/network object. Does that make sense?
  • Simple example code: show express route destructuring the necessary properties from Express req and passing into the business layer scalars and not a req object
  • Anti-pattern example - the opposite from above
  • Comprehenaive example code: I would show an example of a simple app, where there is a folder 'Entry points' that holds all the app doors: API, CLI, etc, and other folders for the internals. I have an example for this
  • Diagram: like this maybe? can probably refine it a bit
    http://snpy.in/7yTgLn

About 'Isolate the domain' - let's discuss soon

Thoughts?

p.s. did you read our content writing guidelines?

@goldbergyoni
Copy link
Owner

/remind me in 5 days

@goldbergyoni
Copy link
Owner

@bikingbadger See my detailed answer above. Shall we have a phone call to discuss this or is it clear enough?

@bikingbadger
Copy link
Author

Hi Natan,
Sorry about the delay. Been bit bogged down at work. I'll add those changes that you proposed and push it hopefully later today.

@goldbergyoni
Copy link
Owner

Sorry about the delay. Been bit bogged down at work. I'll add those changes that you proposed and push it hopefully later today

Sure, at your convenient time. LMK whether a phone call might be helpful (live in Israel too)

@stale
Copy link

stale bot commented Sep 18, 2019

Hello there! 👋
This issue has gone silent. Eerily silent. ⏳
We currently close issues after 100 days of inactivity. It has been 90 days since the last update here.
If needed, you can keep it open by replying here.
Thanks for being a part of the Node.js Best Practices community! 💚

@stale stale bot added the stale label Sep 18, 2019
@goldbergyoni
Copy link
Owner

@bikingbadger Shall we push this?

@stale stale bot removed the stale label Sep 22, 2019
@stale
Copy link

stale bot commented Dec 21, 2019

Hello there! 👋
This issue has gone silent. Eerily silent. ⏳
We currently close issues after 100 days of inactivity. It has been 90 days since the last update here.
If needed, you can keep it open by replying here.
Thanks for being a part of the Node.js Best Practices community! 💚

@stale stale bot added the stale label Dec 21, 2019
@bikingbadger
Copy link
Author

Hi Yoni,
Sorry this one fell completely through the cracks for me. Apologies. Can we look at this in the new year? Do you still think it's relevant?

@stale stale bot removed the stale label Dec 22, 2019
@goldbergyoni
Copy link
Owner

@bikingbadger I think it's highly relevant and will gladly have a chat/phone with you on it

@bikingbadger
Copy link
Author

Great. I have been trying to see where this has disappeared in my repo's for the pull request. I have it locally but I have lost the connection to the PR on my fork. Should I create another PR and close this one and we can continue from there? I'd be happy to talk on the phone, just let me know how to get my number to you or vice versa

@goldbergyoni
Copy link
Owner

@bikingbadger Let start by discussing this by phone and then determine how to put our thoughts on paper. Would you mind shooting me an email to schedule a talk?

https://testjavascript.com/contact-2/

p.s. You coming to the local Node.js conf?

@stale
Copy link

stale bot commented May 16, 2020

Hello there! 👋
This issue has gone silent. Eerily silent. ⏳
We currently close issues after 100 days of inactivity. It has been 90 days since the last update here.
If needed, you can keep it open by replying here.
Thanks for being a part of the Node.js Best Practices community! 💚

@stale stale bot added the stale label May 16, 2020
@stale stale bot closed this May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.2 Layer your components, keep Express within its boundaries
3 participants