[WIP] first draft of client directory and overall coding style docs

[the-vampiire]

have a look and lets develop this PR into a committable first draft

[thinktwice13]

Add jsconfig.json to root

{
    "compilerOptions": {
        "target": "ES6",
        "baseUrl": "."
    },
    "exclude": [
        "node_modules",
        "**/node_modules/*"
    ]
}

But I think target should be ./src. Nothing outside src/ should be imported.

[thinktwice13]

JSX closing tags in new line
Same reasons as trailing commas in multi-line lists

[thinktwice13]

config/
  - keys
  - apollo
  - ...

[the-vampiire]

.eslintrc.js

module.exports = {
    "plugins": ["jest"],
    "extends": "airbnb-base",
    "env": {
        "jest/globals": true
    },
    "rules": {
        "no-console": 0,
        "camelcase": 0,
+        "no-param-reassign": 0,
        "import/no-dynamic-require": 0,
        "import/no-unresolved": [
            2, 
            { caseSensitive: false }
         ]
    }
};

im conflicted over this one

cases:

• express middleware req property injection (which is recommended from their docs)
• mapping over one time use arrays while adding properties
• .then() chains where the source is modified / formed along the chain before being returned at the end

downsides:

• allows mutation of elements in original list if we are not careful

const arr = [{ one: 1 }];
const mapped = arr.map(el => { el.one = 2; return el; });
arr // [{ one: 2 }]
mapped // [{ one: 2}]

alternatives:

• deep clone arguments using _.clone()
  • this will be taxing for large lists

@TheSabby @thinktwice13 @serpient (and your husband if he has an opinion on it!)

[the-vampiire]

here is another example when using Sequelize hooks that would trigger by the eslint rule

ProjectMember.beforeBulkDestroy((options) => {
    options.individualHooks = true;
  });

[thinktwice13]

@the-vampiire This is AirBnB config

    'no-param-reassign': ['error', {
      props: true,
      ignorePropertyModificationsFor: [
        'acc', // for reduce accumulators
        'accumulator', // for reduce accumulators
        'e', // for e.returnvalue
        'ctx', // for Koa routing
        'req', // for Express requests
        'request', // for Express requests
        'res', // for Express responses
        'response', // for Express responses
        '$scope', // for Angular 1 scopes
      ]

It already ignores express req and res. We can add Sequelize hooks here, but I'd rather revert to ESLint default which ignores param properties alltogether and then decide later if we need to make it stricter.

[thinktwice13]

Rules to consider/discuss

• Airbnb also prefers camelCase. We should all discuss that again
• Should we consider operator linebreaks? Currently our ternaries are auto-fixed into inline because we only max enforce line length
• This is a nasty one. We have a lot of click handlers on inappropriate elements
• Max JSX inline props
• ESLint is asking me to destructure params from const { voyageId } = this.props.match.params; Anybody knows the rule to adjust this?

[the-vampiire]

casing

so sabby came up with this pattern because he didnt know js used camelcase lol. but i really like it and use it across all my projects:

• snake_case: all local variables and properties
• camelCase: methods and functions
• PascalCase: classes / models

it makes it really easy to distinguish what you are working with at any given time. and its easier to avoid spelling mistakes due to capitalization for the most common declarations - variables.

the only place it isntt happy with it seems to be on props. but im fine with overriding that for consistency

linebreaks

• ternaries > 80 (100?) chars

return methodName(some_argument, and_another, one_more)
  ? a_true_bit
  : a_false_bit

• long && or ||

  if (
    this_long_thing &&
    one_more_long_part &&
    aThirdOne(called_here)
  ) {
    // do some things
  }

• lengthy JSX (line length or 3+ props)

return (
  <SomeTag
    prop_one = {some_value}
    prop_two = {some_other}
    prop_three = {() => someMethod(stuff)}
  />
);

nasty one

completely agree. we need to write semantic html. at the very least going forward and refactor when you come across previous code

[thinktwice13]

prop_three in SomeTag ends up as a snake_case method :)

But I like the idea.
I'd also add CAPS for constants and enums.

[the-vampiire]

Ah good point. Actually I like bringing the naming pattern to props as well. Variables snake, methods camel, classes (components) pascal. What do you think?

_{Sent with GitHawk}

[the-vampiire]

I can see a case for caps in enums. But not for consts.

On that note think const should be used for everything that won’t be reassigned. 90% of cases are suitable for const. Only use lets during switches or a conditional chain where reassignment is part of the flow.

_{Sent with GitHawk}

[the-vampiire]

Const over let is default in airbnb

_{Sent with GitHawk}

[thinktwice13]

I did not meant local variables, but globally defined app constants, like different options or query/mutation definitions. Redux actions were like that, too. We use those to define filter options and table headers, for example.

…

On Sun, Nov 25, 2018, 04:37 Vamp ***@***.***> wrote: Const over let is default in airbnb Sent with GitHawk <http://githawk.com> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#145 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGTp4eLYiIZG2YawR1IjhrAHUG9RHjPWks5uyhBzgaJpZM4YPxik> .

[thinktwice13]

About bringing the casing patterns to JSX props, I'm good with that if we can enforce it with ESLint. Otherwise I'd rather just stick to one type. I don't care which one (but prefer camel).