Blitz: Uniform code style in monorepo

[aem]

What do you want and why?

We should standardize the code style in the monorepo. As the project grows and we add more functionality, consistency will be crucial in allowing the core team to review PRs efficiently, as well as allowing contributors to move between parts of the codebase without too much friction.

As discussed in Slack, we'd like to start enforcing two rules:

1. Named exports for all non-TSX files.
2. lower-kebab-case file names
3. For single export files, the filename must match the name of the export

For example:

// in cli/src/generators/page-generator.ts
export class PageGenerator extends Generator<PageGeneratorOptions> {
  // ...
}

// in utils/src/log.ts
// filename does not need to match since there are multiple exports,
// but should still be lower-kebab case.
export function info(...) {}
export function warn(...) {}
export function error(...) {}

// in cli/templates/page/index.tsx
// function is a default export since it's a React component, the one
// exception to the named exports rule.
export default function __ModelName__Home(props) {}

Possible implementation(s)

We should make these changes incrementally, they're changes that will almost certainly cause merge conflicts, so the smaller the batches are (maybe one directory at a time) the less disruptive the changes will be.

It may also be worth looking into eslint rules, but only if they're autofixable (mostly for the named exports). eslint rules that aren't autofixable for code style issues (not for rules that are there to prevent bugs) can cause developer overhead that isn't really necessary.

Additional context

Slack thread here: https://blitzjs.slack.com/archives/CUM2C68T1/p1587925285162700

[ryardley]

I think this sort of thing should be done completely by linting rules or not at all. theres this for file names https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/filename-case.md . There is also Kent Dodd’s prettier for file structures idea which might be an interesting concept to throw in the mix although I am not sure if that sort of thing would necessarily work for us

[ryardley]

Maybe this too? https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-default-export.md

[Zeko369]

We should defiantly also add this https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/first.md to keep imports organised, currently we just add them left and right without organising them.

We should keep them

1. Packages
2. Internal packages (core/cli/server)
3. Local files

[jportela]

I agree that this should be enforced by linting rules. I think we should still start by migrating the file names incrementally, to minimize merge conflicts, and then enable the relevant rules. Enabling the rules first would mean that we would only be able to commit the changes all in one batch. I think these two rules are a good start, I'm assigning this to myself to get myself familiarized with the codebase

[jportela]

@aem can you assign this to me? Should I get started on manually changing the files?

[flybayer]

Yeah we start changing files and also add the lint rules. We can set them as warnings until we get everything migrated

[jportela]

setting them as warnings for a start is a good idea, to check on progress (we can always disable the rules if they become too noisy for regular development)

[aem]

We should defiantly also add this benmosher/eslint-plugin-import:docs/rules/first.md@master to keep imports organised, currently we just add them left and right without organising them.

I'm actually pretty against enforcing import order. Having ordered imports doesn't really add anything to the DX in my experience but definitely introduces overhead by forcing developers to fight with the linter. If you find yourself manually updating imports then the other rules have failed, as they're intended to make auto-import work better. For removing unused imports, a quick shift+option+O in VSCode will remove unused imports

[wKovacs64]

There is also Kent Dodd’s prettier for file structures idea

Ben Awad's Destiny does this, but I haven't used it (also not sure I'm keen on the file structure it produces, but it took me a second to warm up to Prettier too and now you'd have to pry it from my cold, dead hands 😉).

Named exports for all non-TSX files.

Out of curiosity, why not named exports across the board? I have no opinion either way, and I know default exports for React components is most common - just wondering if there's a technical reason.

[aem]

Out of curiosity, why not named exports across the board? I have no opinion either way, and I know default exports for React components is most common - just wondering if there's a technical reason.

React has some functionality that requires default exports. Specifically, React.lazy/Suspense only works with defaults. Rather than having special rules for components that we expect to be lazily imported, having default exports for all components seems like a more reasonable middle-ground

[wKovacs64]

React has some functionality that requires default exports. Specifically, React.lazy/Suspense only works with defaults. Rather than having special rules for components that we expect to be lazily imported, having default exports for all components seems like a more reasonable middle-ground

Oh, duh. Makes total sense. Thanks. 👍

[flybayer]

Is this issue done now that #298 is merged?

[jportela]

No, I’m finishing a PR that addresses the rest of the fixes