reversim / reversim-summit-2020

2 stars 2 forks source link

Handling #45 #46

Closed jondot closed 4 years ago

jondot commented 4 years ago

Client: move to yarn, makes better resolution and many security issues go away (mostly were from lodash) Client: shuffle dev deps around, most deps were actually dev. another chunk of security issues go away Client: upgrade axios (the only real runtime security issue) Client: forget about uglifying, we're doing gzip Client: forget about eslint as part of webpack, have eslint in your editors / worst case: CI Infra: add an ignore for 'db' folder just to have mongodb dump stuff there.

rantav commented 4 years ago

Client: forget about eslint as part of webpack

I agree that lint in the editor is recommended and that CI is also recommended, but if both fail (not installed or not configured properly) then what is the last line of defense? The real question is what's the worst that could happen? Would lint errors of any kind result in webpack producing shit? If so then we must have it run on build. If not then I agree it can be remove.

jondot commented 4 years ago

Client: forget about eslint as part of webpack

I agree that lint in the editor is recommended and that CI is also recommended, but if both fail (not installed or not configured properly) then what is the last line of defense? The real question is what's the worst that could happen? Would lint errors of any kind result in webpack producing shit? If so then we must have it run on build. If not then I agree it can be remove.

Linting in this sense is closer to "styles and best practices" rather than closer to "type safety", I'm guessing it's there because the project is a derivate of a create-react-app that was ejected at some point in time. I don't think it would make a substantial difference, this is the first instance of a large project I see with eslint being used in this way, in a blocking path.

jondot commented 4 years ago

Can you please replace npm with yarm in the Makefile as well? And add a short explanation to the readme that yarn is required. Also can you please add the required yarn version in the three package.json files under engines?

Sorry that I didn't mention a small nuance -- I replaced npm with yarn in the client only, careful to leave the backend part as-is, so more of a surgical action there to isolate a problem/solution in once space.

I'm happy to create another branch after this one which isolates replacing the server part of npm w/yarn, including a revamp of dependencies (which will happen naturally since yarn resolves differently)

rantav commented 4 years ago

I'm happy to create another branch after this one which isolates replacing the server part of npm w/yarn, including a revamp of dependencies (which will happen naturally since yarn resolves differently)

ok, cool

rantav commented 4 years ago

lgtm, so are we done?

jondot commented 4 years ago

looks like it, we only need to connect someone's Travis w/this repo and it should start linting don't think i have permissions to add

rantav commented 4 years ago

I'll add travis. Plus I think once you accept the invitation to this repo you'd be able to do so we well ;-)

https://github.com/reversim/reversim-summit-2020/invitations

rantav commented 4 years ago

I had to more to yarn all over, otherwise heroku gets confused and deployment fails https://github.com/reversim/reversim-summit-2020/pull/56

jondot commented 4 years ago

OK i'll look at the lockfile that got created, see if I can spot anything odd.

jondot commented 4 years ago

on a random sample, compared to the npm lockfiles -- looks good.

rantav commented 4 years ago

TBH, I simply used yarn import, that's all...

On Tue, Mar 3, 2020 at 11:12 PM Dotan J. Nahum notifications@github.com wrote:

on a random sample, compared to the npm lockfiles -- looks good.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/reversim/reversim-summit-2020/pull/46?email_source=notifications&email_token=AAAVH7NH72SYBA6CZDXHH43RFVXERA5CNFSM4K7W54Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENVFL6I#issuecomment-594171385, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAVH7PBDHCNE5J32RHNQQDRFVXERANCNFSM4K7W54YQ .

-- /Ran

jondot commented 4 years ago

Perfect. Regarding travis permissions -- just accepted the invite but need org owner perms.. will humbly leave that to you

rantav commented 4 years ago

Yeah that's ok, already done

On Tue, Mar 3, 2020, 23:18 Dotan J. Nahum notifications@github.com wrote:

Perfect. Regarding travis permissions -- just accepted the invite but need org owner perms.. will humbly leave that to you

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/reversim/reversim-summit-2020/pull/46?email_source=notifications&email_token=AAAVH7IIBRVB2DKHMN3PMA3RFVX2NA5CNFSM4K7W54Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENVGABY#issuecomment-594173959, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAVH7LGIMEYF5RL2LS5S2LRFVX2NANCNFSM4K7W54YQ .