Restructure project files

[angelocordon]

Restructure folders to better describe where things go - new proposed changes are as follows:

./
   |_ client/    # Directories for client side code
       |_ components/
       |_ containers/
       |_ hoc/
       |_ pages/
       |_ main.scss
       |_ main.js
       |_ routes.jsx

   |_ api/ # Directory for API/domain-specific models that are imported in both client and server
       |_ entries/ # Entry directory, contains model for Entries, methods, publications and helpers

  |_ lib/ # Directory for shareable code that is not domain-specific
      |_ constants/
      |_ data/

  |_ server/ # Directory for server-side business logic and database
  |_ specs/ # Directory for e2e tests; NOTE: Unit tests are self-contained within their own components

The goal here is that folders are semantically named and show where code and functionality need to go based on what area of the app it touches.

[d3vild06]

@angelocordon I am seeing the error below on the landing page:

Are we sure we're expecting an array for this prop?

[angelocordon]

Good catch @d3vild06, I'll look into it.

[angelocordon]

👍 Fixed up the issue, though I'm looking to deprecate the Board component real soon @d3vild06

[distalx]

While we are at it, I'm proposing following directory structure - which is more flat in my opinion.

cb-connect
└─── .meteor
└─── client // all the things related to client side. (Components, Styles, Tests ...)
│   └───components
│   └───pages
│   └───stores
│   └───tests
│   main.js  
│   ...
└───lib // code which is being used by both side. ( if we like we can rename this to both )
│   ...
└───public
│   ...
└───server // all the codes (Methods, Publications, configs
│   └───api
│   │   └───entries
│   │   └───users
│   └───configs
│   └───seeder
│   └───tests
│   main.js  
│   ...
└───specs

Since, meteor has introduce mainModule api. we can define our entry point using that. That was not possible earlier and people come-up with imports directory pattern to achieve that behaviour. But now imports directory is no-longer needed and we can have more flatter directory structure.

Please checkout our master branch for more reference. we didn't had imports directory in initial commit. https://github.com/codebuddies/cb-connect/tree/master.

Personally, I find easy to navigate with this structure. Like if it's Front-end related issue you can find all the file under client, and same goes for server-side issue.

What's your opinion? @angelocordon, @d3vild06, @gauravchl, @lpatmo.

[d3vild06]

I like it. A folder structure I saw in other projects were self containing components where the test, css and jsx files were all together in the same component folder.

I thought that made it easier to reason about and manage. Any thoughts on this structure?

[angelocordon]

I like it - my few points are:

• as per Roberto’s comment, I like having specs inside the same component folder as well as everything is self contained - when working on a component, things are all in one place (jsx, scss, and spec files). Outside of e2e tests, which should probably go in the root specs folder, this makes sense.

• The api directory, which I’ve been seeing as where all our models go, would it be better to have it in the root? My thoughts here are that classes and their methods can be imported in the client side also, but they’re not really libs. (Hmm, now that I’m typing this out, it feels a little sketchy being able to import models in the client end - it’s really close to being able to do business logic in the front end, which... feels very close to spaghetti code)

[distalx]

Sweet!

Re specs

if we keep specs inside our components directory, we won't need the test directory inside clients. It would look as follow....

cart_info.jsx cart_info.test.jsx

Right?

RE: API

We can keep model definition inside 'libs' directory.

But we might want to keep 'methods' and 'publications' inside server directory.

On Sat, 4 May, 2019, 9:57 PM Angelo Cordon <notifications@github.com wrote:

I like it - my few points are:

-

as per Roberto’s comment, I like having specs inside the same component folder as well as everything is self contained - when working on a component, things are all in one place (jsx, scss, and spec files). Outside of e2e tests, which should probably go in the root specs folder, this makes sense.

The api directory, which I’ve been seeing as where all our models go, would it be better to have it in the root? My thoughts here are that classes and their methods can be imported in the client side also, but they’re not really libs. (Hmm, now that I’m typing this out, it feels a little sketchy being able to import models in the client end - it’s really close to being able to do business logic in the front end, which... feels very close to spaghetti code)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/codebuddies/cb-connect/pull/73#issuecomment-489341793, or mute the thread https://github.com/notifications/unsubscribe-auth/ACVM3QG5NL4GI7F6GJVEW43PTW2NXANCNFSM4HJWQEHQ .

[d3vild06]

Yea I'd expect the structure to look like

├── components
│   └── navbar
│       └── navbar.jsx
│       └── navbar_test.jsx
│       └── navbar.scss

Each component is self-contained within its own directory. However, I just remembered we are using Cypress as our testing tool right? I am not sure how to make this work with Cypress though. Does it support this structure? 🤔

[angelocordon]

That's right @d3vild06 - these component tests are most likely going to be unit tests. The other thing I think would probably happen is that Jest would add a snapshots folder in these component folders for snapshopt testing. I think it might end up looking something like:

components/
|__ navbar/
     |__ __snapshots__/
          |__ navbar.spec.js.snap
     |__ index.jsx 
     |__ navbar.spec.js
     |__ navbar.scss // or css.js for CSS modules

I personally like index for the component files only so that we can import components as:

import Navbar from 'components/navbar';

vs

import Navbar from 'components/navbar/navbar';

A quick note on naming conventions - I'm not married to any of what I proposed to above, but let's settle on one and commit to consistency. Our only constraint so far is how Jest looks for any js files with spec or test in its file name. IIRC, I think it's either JS files under a specs or tests directory or file.spec.js or file.test.js regardless of where they live.

--

re: Cypress, I can't really see how any of those kinds of tests would fit within the component directories themselves, hence I figured we'd still leave a root level test or spec directory specifically for e2e or any other kinds of tests outside of unit tests.

--

Re: api in libs: My personal understanding is that libs is really more for custom libraries (non vendor libraries like lodash), and less for app-specific files like (models, controllers). Is this something different in Meteor?

I found this article from CodeClimate talking about the same thing: https://codeclimate.com/blog/what-code-goes-in-the-lib-directory/

Should collections, methods and publications be self contained within the same models?

|__ api/
    |__ entries/
          |__ index.js // main Entry entry point
          |__ entries.js // main Entry model for methods, validations, etc.
          |__ entries_collection.js // Defines Entry DB Collection
          |__ entries_publication.js

[d3vild06]

@angelocordon I believe the lib directory is eager loaded by Meteor. So anything we'd want to ensure is loaded before our application is bootstrapped would go into this directory. Not sure if that's why other Meteor projects took the approach of defining the collections/methods under the lib directory to ensure they were available to the application at startup.

In any case, I believe this can be mitigated by ensuring we define two entry points for both server & client (client/main.js and server/main.js) that will be eager loaded and we can still define methods/collections under those directories with no problem. I believe we're already doing this anyways.

@distalx any other thoughts on moving collections/methods outside of the lib directory?

@angelocordon I agree with sticking to a naming convention and utilizing the index.js for each component as the entry point. That does provide a cleaner import syntax.

According to the Meteor guide, it appears it encourages self-contained publications and methods so I see no issues with following a similar convention. I think self-containing them to a unit of domain logic (i.e entries) makes a ton of sense.

[distalx]

RE: methods and publications.

Anything which is not inside a client or a server directory meteor considers that as a shared code. It will be available on client and also on the server.

The code which is required on server and on client, people used to keep that code either inside lib directory or both directory. The directory name don't really matter to Meteor.

The Meteor.methods or publications defined outside the server directory was also bundled into client side code. meaning by executing following command in browser console someone can figure-out business logic.

// this will return a method body.
Meteor.connection._methodHandlers[<"method_name">].toString()
// this will return publications
Meteor.connection._subscriptions

Even meteor guide they have also kept the publication inside server directory for the this reason, just an extra layer of precaution. Check out the Example directory layout.

To avoid this kind of accidental exposer, it is advisable that either keep sensitive code inside a server directory or define that code block inside Meteor.isServer. That is the norm in meteor community. And this was the reason I suggested to keep the methods and publications inside server directory.

After doing some reading and experimenting. I figured out that use of meteor.mainModule completely overrides meteor’s default rules for eagerly loading modules. So we can keep them wherever we see fit. We just have to make sure that we don't accidentally import them to client.

RE : utilizing the index.js for each component.

This might sound nitpicky, but I find it difficult to navigate/switch between open files if they all have similar name. like most of the open tab would indicate index.js -. Do you folks have this issue? Maybe I'm using the wrong editor. At the same time I also see the point that it give the cleaner import statements.

[angelocordon]

Thanks for sharing @distalx - those are great insights.

The Meteor.methods or publications defined outside the server directory was also bundled into client side code. meaning by executing following command in browser console someone can figure-out business logic.

If so, I think it makes sense to keep the api directory in the server directory then. I think this would also give us an opportunity to separate concerns where our backend should be concerned only about serving the right data structure and our frontend should only be concerned with client-based logic/rendering data.

I suppose the only kinds of methods we'd need access to in the client side are methods that update collections, which if needed, we can export/import helper methods -- does that make sense to you both? @distalx @d3vild06

-- I haven't had an issue with the index.js as a naming convention -- tbh, I've confused files before regardless of what they're named, so I'm not really any help there 🤷‍♂

[d3vild06]

I don't think we technically need to expose any client side methods that update collections. The only reason I'd see us needing or wanting that is for optimistic UI updates which I think Meteor provides OOB with mini Mongo right?

In terms of the file naming convention and working with an index file. Chances are I'm often working on the component JS file itself (which I'd fuzzy search for inside my code editor) and not the index file so I'm not really seeing the issue with this approach. My editor tabs would clearly see what components files I have open.

In the interest of trying to close out this PR, do we want to move forward with the proposed changes @angelocordon and commit them for a final review?

[angelocordon]

Sounds good - I'm going to go ahead and make those suggested changes in this PR 🙌 Thanks for the discussion all.

[distalx]

fo’ shizzle 👍

On Wed, May 8, 2019 at 12:22 AM Angelo Cordon notifications@github.com wrote:

Sounds good - I'm going to go ahead and make those suggested changes in this PR 🙌 Thanks for the discussion all.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/codebuddies/cb-connect/pull/73#issuecomment-490211278, or mute the thread https://github.com/notifications/unsubscribe-auth/ACVM3QHI4TKF7DQ2EZPLPWTPUHFVTANCNFSM4HJWQEHQ .

[lpatmo]

I took a quick look; LGTM!