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

RFC for Blitz App File Structure #3

Merged
merged 12 commits into from
Apr 2, 2020
Merged

RFC for Blitz App File Structure #3

merged 12 commits into from
Apr 2, 2020

Conversation

flybayer
Copy link
Member

@flybayer flybayer commented Feb 28, 2020

@ryardley
Copy link

ryardley commented Mar 2, 2020

The file structure of a project is something that adds zero value to your app. Therefore time spent debating or managing this is truly a waste of time.

I think I disagree with this. Perhaps you mean "The file structure of a project is something that adds zero value to your users"?

File structure in any app must not be arbitrary or you end up with code that is impossible to work on and maintain.

Good file structure can:

  • Communicate a mental model of how an application works.
  • Help designate and separate related dependencies.
  • Act as metadata. (eg. filesystem routing)
  • Signal intent
  • Designate execution environments (eg. client/server)
  • Enable easy relocation and refactoring of related entities.
  • Flag areas of code for change / consideration

... and other things I cannot think of right now.

@ryardley
Copy link

ryardley commented Mar 2, 2020

Some initial thoughts on this. Some of this might be mitigated by a bit more clarity on how we are managing dependency injection. I feel like having a firm understanding of the magic we are planning for blitz might inform file structure and vice versa.

  • /pages already has conceptual dependency associated with its controller which is inside the app context and in my opinion would be more useful being available closer to that context as well as at the root. Blitz could then merge all the pages together and spit the dummy if there are any dupes. This would be an optional folder.
  • If you try and create separate /layout for a specific app context (eg. the "users" section.) You will create a far flung dependency relationship there so it makes sense to hold layouts for app contexts close to the context. Again and optional folder.
  • Often I don't want to promote a util to a global scope until it has more than a single dependency. So local utils would be well placed to exist within an app context as well. Alternatively limiting to a global utils store might make sense if blitz could simply warn on unused utils which might be a fair compromise.
  • Blitz will wrap next and will undoubtedly require its own configuration file. Perhaps we should have the blitz config contain the next config or, alternatively we could have a config/ folder hosting global configuration

So I guess from my suggested changes might look something like this:

├── app/
│   └── users/
│       ├── components/
│       │   └── Form.js
│       ├── pages/ (optional)
│       │   └── api/
│       ├── layouts/ (optional)
│       │   └── SpecialUserLayout.js
│       ├── controller.js
│       ├── model.js
│       ├── tests/
│       └── utils/ (optional)
├── db/
│   ├── migrations/
│   └── schema.prisma
├── integrations/
├── jobs/
├── layouts/
│   ├── Authenticated.js
│   └── Public.js
├── conf/
│   └── next.config.js
├── blitz.config.js
├── pages/
│   └── api/
├── public/
│   └── favicon.ico
├── tests/
└── utils/

@flybayer
Copy link
Member Author

flybayer commented Mar 2, 2020

I think I disagree with this. Perhaps you mean "The file structure of a project is something that adds zero value to your users"?

File structure in any app must not be arbitrary or you end up with code that is impossible to work on and maintain.

Good file structure can:

Communicate a mental model of how an application works.
Help designate and separate related dependencies.
Act as metadata. (eg. filesystem routing)
Signal intent
Designate execution environments (eg. client/server)
Enable easy relocation and refactoring of related entities.
Flag areas of code for change / consideration
... and other things I cannot think of right now.

Yes, you're exactly right. And I love those points on file structure. They will be very good to add to the RFC.

@aem
Copy link
Collaborator

aem commented Mar 2, 2020

I'm mostly curious why we're logically separating pages from the rest of the components of a view (app/myView), as well as why we're forcing deeply nested file paths. My general belief is that all related code should be colocated and instead apps should enforce strong naming conventions. e.g.

├── app/
│   └── users/
│   │   └── UsersController.js
│   │   └── UsersView.js
│   │   └── UsersModel.js
│   │   └── UsersController.js
│   │   └── UsersApi.js

And in the build tools we specify that we'll only look for files that match the pattern ${folder.toTitleCase()}${Component}, we can allow users to pick whatever folder structure they'd like (we don't care what the files in the /users directory import, that's webpack's job to handle), as long as those 5 files exist in a place where we're expecting with a name we're expecting. It has other ancillary benefits like making all imports local (no ../../some/long/wild/path imports), and allows for

Since the Blitz context is kind of abstract since we're still iterating on this, I can use a standard React/Redux app as an example. I find this:

├── redux/
│   └── usersReducer.js
│   └── usersActions.js
├── data/
│   └── usersModel.js
├── api/
│   └── usersApi.js
├── components/
│   └── User.js
├── containers/
│   └── UsersContainer.js

...significantly harder to grok (especially for newcomers) than this:

├── users/
│   └── usersReducer.js
│   └── usersActions.js
│   └── usersModel.js
│   └── usersApi.js
│   └── User.js
│   └── UsersContainer.js

...which, at a glance, gives a very clear picture of what's happening, how different files interact with each other, etc. IMO, as long as good naming conventions are followed (something we can enforce in our build tooling), it makes understanding the full set of files that make up a page much easier. It does have the advantage of also making integration tests much easier to manage, if a test breaks or has to be updated, you have a single place to look, no need to go spelunking across your entire repository.

@ryardley
Copy link

ryardley commented Mar 2, 2020

I'm mostly curious why we're logically separating pages from the rest of the components of a view (app/myView), as well as why we're forcing deeply nested file paths.

This is a convention from Next.js. I agree it shouldn't be separated.

@flybayer
Copy link
Member Author

flybayer commented Mar 2, 2020

@aem I agree on all "user" files living in the same place. My initial proposal was this, with the exception of a components folder. Pages/views aren't in here because Next doesn't support that, but @ryardley is exploring possibilities for this.

Quoted for reference:

├── app/
│   └── users/
│       ├── components/
│       │   └── Form.js
│       ├── controller.js
│       ├── model.js
│       └── tests/

Now for the actual file naming, I'll be honest, I don't like the verbosity of duplicating the folder name inside the file name (users/UserModel.js), but I'm not totally opposed to it so let's have a discussion on it.

Why do you want users/UserModel.js instead of users/model.js?

@aem
Copy link
Collaborator

aem commented Mar 2, 2020

This is a convention from Next.js. I agree it shouldn't be separated

Is our goal for users to have to know about Next? If we'd like them to be aware of the underlying framework then that's fine, but maybe we go with the flat file structure everywhere except the pages directory, and pages is the only logically separate folder? If not, and we'd like it to just be an implementation detail, then we could get creative and have the first step in blitz build (or whatever the name ends up being) be to copy the project into a temp directory, move the *Page.js files into /pages, and then we can use a custom file watcher than knows how to apply changes from the working directory into the temp directory to keep dev reloading working properly.

Or, maybe for more transparency, as a part of the build we generate pages/ entries for the user. Detect all *Page.js files under /app, and for each one, create a file in /pages with the contents:

// THIS IS A BLITZ AUTO-GENERATED FILE, ANY CHANGES WILL BE OVERWRITTEN
import MyPage from '../app/mything/MyPage.js;
export default MyPage;

@ryardley
Copy link

ryardley commented Mar 2, 2020

Or, maybe for more transparency, as a part of the build we generate pages/ entries for the user

@aem That is a technique I have used before with next. I am looking into setting this up in an example over the next few days that uses this sort of thing. It should allow us to use next under the hood but not be bound by some of its design flaws.

@aem
Copy link
Collaborator

aem commented Mar 2, 2020

Why do you want users/UserModel.js instead of users/model.js?

The simpler model.js option, while easier to type and saves some keystrokes, has some scale problems as far as developer experience is concerned. We tried this at my day job and moved away from it because as the project grows, finding files becomes impossible. We have a concept of "cards" in our app, and each card has a Header, Content, and a few other things. Here's what it's like trying to find the file you're looking for:

image

While you can type the file path to filter down the file list, because of how IDEs' search relevance works, the actual file you need might be a few entries down the list. While the verbosity isn't ideal, if your file is just named usersModel.js, typing in usersModel will always make it the first result, and easy to find.

@flybayer
Copy link
Member Author

flybayer commented Mar 3, 2020

Alright, great point let's do that. So,

  1. Should the folder name (app/users) be singular or plural?

I initially went with plural because routes are plural. But if the folder name is now in the file name, I think singular would read and look better. And it could be a nice distinguisher from routes. Routes are plural, code is singular.

userModel.js, userController.js

For reference, Ruby on Rails has singular models, plural controllers, and plural routes.

  1. What should the spelling case be?

I think all React components should always be PascalCase, but for app files:

userModel.js
UserModel.js -> if components are PascalCase, then can't use this
user-model.js
user_model.js

My vote is for kebab-case.

@ryardley
Copy link

ryardley commented Mar 3, 2020

Ultimately it doesn't matter so long as it is consistent but a couple of points worth mentioning:

  1. React components might already distinguishable based on filename extension.
  2. There is an argument for efficiency of being able to copy from a filename to an import object:
import CandidateApplicationController from '../CandidateApplicationController';

@flybayer
Copy link
Member Author

flybayer commented Mar 3, 2020

@ryardley point 1 is true for TS projects, but not JS projects which we'll certainly have.

@richardcrng
Copy link

Should users have to know about Next?

I suggest that the goal should be, explicitly, users should not need any knowledge of Next, since I think it'll be better for the long-term:

  1. if when Blitz is the default for building full-stack React apps, we might expect some developers entering the ecosystem for the first time to start using Blitz without ever having to know about Next
  2. speculatively, do we know that Blitz is always and forever going to be using Next? I'm viewing Next as an implementation detail which a lot of entry-level users might never need to know about, and which we theoretically would be able to change without them knowing

Expanding a bit on the first of these (with based off observations about Redux and forecasting parallels for Blitz - I've never actually used Next before):

As a brief summary of what I can reconstruct from the historical record: Redux was written as one of the many 'Flux' libraries that popped up, and followed Flux conventions in doing so, e.g. opting for the terminology of 'actions', with a view to being easier for those familiar with Flux.

Since time has gone on, a significant proportion of users - probably the overwhelming majority, in the maintainer's estimate - are now coming to Redux without the historical knowledge of Flux.

The Redux documentation and API, originally perhaps written for a Flux-literate audience to get them to choose Redux over other Flux library, weren't optimised to work for users arriving into the ecosystem later on (understandably). Newcomers are impatient and just want to learn this library that everybody keeps talking about - the "learn the concepts of this strange thing called Flux which you have never heard of before, and then you can learn this library you really want to learn" behaviour is difficult to motivate and guide.

I think this is likely to be one of the main reasons why Redux has gained a bit of a reputation as being hard to learn.

Additionally, there's some terminology baggage that has come with the Flux roots. There's been discussion on whether Redux 'actions' should really be called 'events' - and the official Redux recommendation is now that actions are better modelled in an event-style, rather than the imperative command style that most devs use.

They're sticking with the 'actions' terminology (which I think is sensible - it's too late to change it now), but that terminology does lead people down the imperative command style path which was not intended.

I don't know if similar things will arise with Blitz, but I think it's something to be conscious of.

I reckon that the rate of new people coming into web development is large enough that "people gingerly stepping out into the world of programming" is worth considering as an important audience who need to be able to work with Blitz.

File location

I find this:

├── redux/
│   └── usersReducer.js
│   └── usersActions.js
├── data/
│   └── usersModel.js
├── api/
│   └── usersApi.js
├── components/
│   └── User.js
├── containers/
│   └── UsersContainer.js

...significantly harder to grok (especially for newcomers) than this:

├── users/
│   └── usersReducer.js
│   └── usersActions.js
│   └── usersModel.js
│   └── usersApi.js
│   └── User.js
│   └── UsersContainer.js

...which, at a glance, gives a very clear picture of what's happening, how different files interact with each other, etc.

I agree with the principle of co-locating together files that change frequently together, or which are conceptually related - as my two-cents, though, on this comment from the Slack group:

I tend to ask "I am in the backend or am I in the frontend?"
and I like a file structure that helps answer that quickly

I am a fan of this, and I think that's not as immediately achieved by the second Redux pattern - so I'd personally do something more like grouping User.js and UsersContainer.js in the same folder on one part of the tree, and usersReducer.js, usersActions.js, usersModel.js and usersApi.js grouped together in a different part of the tree, to separate out frontend and backend conceptually via folder structure.

An additional advantage of this, to my mind, is that putting all user-related files (frontend and backend) in the same folder seems to imply, or at least suggest, that it is only these particular components in this folder that ought to be interacting with this part of the backend API, and so I feel a bit dirty if I dispatch an action to change user state from a different component - but I think that might be legitimate?

@flybayer
Copy link
Member Author

flybayer commented Mar 4, 2020

Great thoughts @richardcrng!

I think you're suggesting something like this, correct?

components/
  user/
    User.js
    UserContainer.js
    UserForm.js
    UserList.js
    pages/
      ...(user pages)
data/
  user/
    UserController.js
    UserModel.js
    api/
      ...(user api routes)

Both components & data would support nested contexts.

@richardcrng
Copy link

Yes, exactly @flybayer

@aem
Copy link
Collaborator

aem commented Mar 4, 2020

I still feel as though that's being too prescriptive about what the file structure should look like. If we just dictate that "every folder needs to have these n files in it for models, controllers, etc." it lets the user be 100% in control of how they organize their code. By us requiring data and api and pages directories we're locking them into a structure that might not mimic how they think about their code.

@quirk0o
Copy link

quirk0o commented Mar 4, 2020

I know this might be an unpopular opinion but how do you feel about extending the "files that change together should live together" rule to tests? In a very large project I've always found the convention to put UsersController.js and UsersController.test.js right next to each other much easier to navigate (although I do use file nesting in my IDE so I might be biased)

@flybayer
Copy link
Member Author

flybayer commented Mar 4, 2020

@aem we have to have pages and api directories unless we use configuration based routing instead of file-structure routing, right?

So far, I don't see any actual requirements to follow this file structure (aside from having pages and api. All files still have to explicitly import from other files. This fill structure will be used by the blitz generate command, but you can skip that or only generate a few pieces etc.

I strongly think we need a scalable file structure by default as the recommended path. "How should I organize my react app files?" is one of the most common questions junior/mid people have. Blitz is in the business of eliminating mundane decisions like this.

But at the same time, if at all possible, one should be able to organize files in their own custom way.

@flybayer
Copy link
Member Author

flybayer commented Mar 4, 2020

@quirk0o yeah — I did a survey on twitter the other week, and the vast majority of people have a top level tests folder for integration tests but then use thing.test.js for unit tests.

@aem
Copy link
Collaborator

aem commented Mar 4, 2020

we have to have pages and api directories unless we use configuration based routing instead of file-structure routing, right?

Not if we use the POC where we use some basic codegen and files are served from a temporary .blitz directory. If Next.js is an implementation detail, we can make the file structure look however we want and just update our build tools to reflect those requirements. By saying that "any APIs must be served out of /resource/path/*Api.js then *Api.js can look however the user wants. If it just does an export { api } from './api/myActualApi.js' then the user is more than welcome, but I'm not sure it makes sense to tie someone to a deeply-nested folder structure if they're writing a very simple API that only requires a few lines of code

@flybayer
Copy link
Member Author

flybayer commented Mar 4, 2020

@aem I get what you're saying, but I'm not following the solution. With the *Api.js approach, exactly how do you define the routes?

I.e.

/things
/things/:id
/things/:id/otherthing
/something-custom-for-thing

@ryardley
Copy link

ryardley commented Mar 6, 2020

One thing I think I like that is worth mentioning around file structure was that whilst I was playing around with the implementation of the "compile to next" idea I discovered we could probably set up Blitz so that you can simply decorate a working next app and "upgrade" it to blitz. Trying to make this possible would be quite worthwhile as it could see much larger adoption of blitz as it should make it easier for people to gradually adopt blitz into their next projects to handle their data fetching. So in my head what that means for file structure is that :

  1. We should keep the /pages terminology as default including the /pages/api structure (or make it configurable in blitz.config.js to be something the user likes like /routes and /app/users/routes/user.ts and /app/users/routes/api/user.ts etc.)
  2. We should ensure next.config.js and similar files like now.json etc. are exactly where you would expect them to be and not in a /conf folder or anything like that.
  3. We don't need to try and "hide" next at all

Upgrade could be easy manually: just install npm install blitz and use the config decorator import {withBlitz} from "blitz/config" around the next config. then change your package.json scripts to blitz start blitz build etc.

Also generally one of the things I like about React and annoys me about more prescriptive systems is that you can organise them however you want. Having a canonical way is super important to eliminate decision fatigue but being able to do whatever you want is important too. Ultimately the user should be able to configure how the pages folders are found to taylor it for their needs.

@aem we have to have pages and api directories unless we use configuration based routing instead of file-structure routing, right?

Being able to do configuration based routing could be super useful too btw. With large apps a file system based approach can be unwieldy to have the option of either will be well received I think.

@flybayer
Copy link
Member Author

flybayer commented Mar 6, 2020

@ryardley yeah, I think that's a really good idea. I want it to be as easy as possible for folks to migrate a next app to Blitz.

  • I think we could still use routes instead of pages. It's very easy to just rename the folder, and Next is planning to make this change anyways.
  • For config, we could have blitz.config.js that's the same structure as next.config.js but without needing the withBlitz wrapper. Again, very easy to just rename the file and everything still works.

@ryardley
Copy link

ryardley commented Mar 6, 2020

Next is planning to make this change anyways.

I guess that means they will be supporting both or will they be making a breaking change?

@flybayer
Copy link
Member Author

flybayer commented Mar 6, 2020

I'm sure they'll support both for some time, so we probably should too, eh? :)

@ryardley
Copy link

ryardley commented Mar 6, 2020

I'm sure they'll support both for some time, so we probably should too, eh? :)

Sounds like a good default starting point. I still think it would be good to have this configurable at some point. /views could be another candidate people might want but it probably should not be default.

We still would need to select a canonical name for the generator so the config might be something like:

{
  routeFolderName: '✨',
  routeFolderMatcher: /(\/(pages|routes|views|✨)\/.+)$/
}

NOTE: not the default config 😉

@flybayer flybayer mentioned this pull request Apr 1, 2020
@flybayer
Copy link
Member Author

flybayer commented Apr 1, 2020

I've made some major updates to this file structure RFC, including rolling the routing RFC into this since they are so closely related.

Please review and provide feedback! It certainly still needs more work and explanation, but it's making progress.

@flybayer flybayer merged commit cb5a977 into master Apr 2, 2020
@flybayer flybayer deleted the files branch April 2, 2020 12:08
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.

5 participants