ESLint & Prettier POC

[avatarneil]

Working towards #100.

Copy-pasta'd some ESLint & Prettier configs from another project, and resolved all linter errors in the express app. Also applied prettier to the react app. Need to figure out why ESLint isn't picking up anything in the React app before this can be merged, so for now this will just be a draft.

[kelvinfan001]

Re: the React app, I think create-react-app already has some sort of ESLint configured, not sure if this may be causing issues with the ESLint configured in the root dir.

http://rahulgaba.com/front-end/2019/02/17/Use-custom-eslint-config-in-create-react-app-using-three-simple-steps-No-external-dependencies.html

[kelvinfan001]

ESLint isn't picking up anything in the React app

This is probably due to https://github.com/kelvinfan001/findalicious/blob/a14cb6be254a3de97147d9a6a02cfbe30c4b6b5f/react/package.json#L30. I think it might be overriding the ESLint Config in the root dir. If we remove this, then I believe the listing rules from the new .eslintrc.json in the root dir would be applied. However, a ton of weird (incorrect?) errors would be spat out by the linter (related). I'm not sure what's in the "react-app" ESLint configs, but Facebook recommends us to extend that, and that seems to not spit out all the weird errors. For now, a workaround to have some basic code style detection is to simply add "plugin:prettier/recommended" to this item. We might eventually need to maintain two sets of .eslintrc.jsons so we can extend their "react-app" configs in the /react dir. Do you have any additional insight into this @avatarneil ?

I'm fine with just using Create React App's configs in /react and use prettier alongside it, but maybe that's because I'm not familiar with the config you added and not aware of the additional benefits it brings.

[avatarneil]

We might eventually need to maintain two sets of .eslintrc.jsons so we can extend their "react-app" configs in the /react dir. Do you have any additional insight into this @avatarneil ?

@kelvinfan001 I think there's a very good chance we'll have to do this. I'll look into bringing in ESLint configs geared for the FE that extend the existing config. Should have an update for the PR tomorrow or Tuesday 🙏

[kelvinfan001]

Sounds good. Thanks for working on this! I learned a lot.

[avatarneil]

Getting the react ESLint config set up is taking a bit longer than anticipated (mostly me digging through the errors and seeing if they're for rules we want to keep), but I'm making good progress. Should hopefully have and update in the next few days 🙏🏼

[kelvinfan001]

Haha no rush at all! I’ve been pretty busy with work as well, so I haven’t been able to contribute as much as I would like to. Please let me know if I could help, though!

[kelvinfan001]

Apologies for intruding, just saw a new commit and wanted to test things out :) Things seem to be linting properly on my end as well. Another thing we would need to sort out with this new directory structure (where the backend's package.json isn't in the project's root directory) is how we deploy with Heroku. Haven't tinkered with this in a while, but I believe Heroku makes it a lot less ergonomic to deploy when it can't find a package.json in the project's root directory, which is why we currently have this somewhat unnatural structure. I'll try to figure this out before we merge this, hopefully it's not too bad. Thanks again for all this work, and just remove the draft label when you're ready and I'll take another closer look!

[avatarneil]

@kelvinfan001 Absolutely not intruding! You're correct, we definitely need to test Heroku deployments before this gets anywhere near master. I need to test that out, plus resolve some merge conflicts before this PR is totally ready. I wonder if we want to introduce something like Yarn Workspaces or Lerna along with these changes, considering I think we'll likely have to move things out of the root dir and into child dirs, but perhaps the introduction is better suited for a subsequent PR.

[avatarneil]

I merged in master, and for whatever reason Github still seems to be unhappy... We may need to do a manual merge for this PR 😭. @kelvinfan001 everything seems to be working on my machine w/ docker-compose, though I have yet to try deploying to Heroku; do you want to try to deploy?

[kelvinfan001]

ah, let me see if I can resolve those conflicts.

Edit: oh wow, yes the conflicts are quite tricky...

[kelvinfan001]

I tried to do the merge manually, but it was quite difficult. I think this is mainly due to the immense amount of style errors that were automatically fixed by the linter. I think it might be simplest to checkout a new branch off of the most up-to-date master, commit only the new linter config files, commit the changing of the directory structure (renames), then commit the linter's automatic fixes. Finally submit a new PR for this new branch. What do you think? I probably contributed a lot to the mess, I was pushing some structural changes to master while you were working on this PR and did a force-push in an attempt to lower the number of merge conflicts (actually made it ten times worse I think), really sorry about that haha. Anyways, I do think a huge reason for all these conflicts was because the automatic fixes by the linter weren't in a separate commit. What would've been ideal was for this PR I think would've been to do everything EXCEPT the automatic fixes, and then once we're ready, stop all development and changes to master, add a commit to do the auto fixes, then merge into master immediately.

[avatarneil]

@kelvinfan001 I think your read is spot on, and agree with all points. I'll cherry-pick the PR, cut a new PR for the linter/formatter files and reorg, then cut a fixes PR on top of it.

[kelvinfan001]

Thanks so much, this is a huge PR, and especially tricky with all those linter changes! Really sorry for adding to the mess.

[kelvinfan001]

do you want to try to deploy?

Ok looks like deploying a node app where package.json isn't in the root dir to Heroku isn't too bad at all (albeit somewhat unintuitive). I think I've gotten it to work.

[kelvinfan001]

Closing in favour of #110