Page MenuHomePhabricator

[lib,web,native,landing] [chore] update eslint config to correctly handle cjs files
ClosedPublic

Authored by ashoat on Apr 18 2022, 12:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 1:52 PM
Unknown Object (File)
Fri, Nov 15, 6:09 AM
Unknown Object (File)
Sun, Nov 10, 12:39 AM
Unknown Object (File)
Fri, Nov 8, 8:21 PM
Unknown Object (File)
Fri, Nov 8, 8:21 PM
Unknown Object (File)
Fri, Nov 8, 8:21 PM
Unknown Object (File)
Fri, Nov 8, 8:21 PM
Unknown Object (File)
Fri, Nov 8, 8:21 PM

Details

Summary

update eslint config to ignore files, rather than add the comments, https://eslint.org/docs/user-guide/configuring/ignoring-code

Test Plan

go to webpack files, red swiggles are now gone.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested changes to this revision.Apr 18 2022, 8:38 PM

How about lib/webpack/shared.cjs? Furthermore: if the ESLint parser simply can't handle CommonJS, should we just disable it for all .cjs files?

Is there a way to get this to work by configuring ESLint to treat .cjs files as CommonJS? Have you tried /* eslint-env commonjs */ annotation, or any of the other strategies described here?

This revision now requires changes to proceed.Apr 18 2022, 8:38 PM

How about lib/webpack/shared.cjs? Furthermore: if the ESLint parser simply can't handle CommonJS, should we just disable it for all .cjs files?

Is there a way to get this to work by configuring ESLint to treat .cjs files as CommonJS? Have you tried /* eslint-env commonjs */ annotation, or any of the other strategies described here?

I tried: /* eslint-env commonjs */ annotation but that didn't work.

I think what makes the most sense is having the the

/* eslint-disable no-undef */
/* eslint-disable flowtype/require-valid-file-annotation */

annotation. If we just completely ignorePatterns the file like I had before eslint isn't enforcing anything. Using the disable no-undef and flow type we're just disabling features we don't need / use.

add annotations to webpack files

ashoat requested changes to this revision.Apr 22 2022, 6:25 AM

Let's try to go through this in person at some point. You may be right but I want to step through it with you and make sure there isn't a better solution here. Can you remind on Monday? (Or alternately, feel free to preemptively book some time on Monday)

This revision now requires changes to proceed.Apr 22 2022, 6:25 AM

Update to use ESLint overrides config to set .cjs files to commonjs and node env, and to disable Flow rules as well

ashoat retitled this revision from [lib,web,native,landing] [chore] update eslint config to ignore webpack files to [lib,web,native,landing] [chore] update eslint config to correctly handle cjs files.
This revision is now accepted and ready to land.Apr 25 2022, 9:05 AM

@benschac, keep in mind when landing this that you'll need to arc patch to get my latest revision. (Alternately, if you prefer I can commandeer, you can accept, and then I can land)

@ashoat, you should probably commandeer at this point. You did the implementation while I watched. Happy to take it if that's easier for you though.

ashoat edited reviewers, added: benschac; removed: ashoat.

Cool, just need an accept then

This revision now requires review to proceed.Apr 25 2022, 12:32 PM
This revision is now accepted and ready to land.Apr 25 2022, 1:45 PM