webcompat / webcompat-reporter-extensions

Browser extensions to help report site compatibilty issues.
26 stars 21 forks source link

Fixes #126. Consolidate webpack configuration into a single file. #127

Closed laghee closed 6 years ago

laghee commented 6 years ago

From preliminary "testing," it looks like this will work. I didn't want to mess around too much with the existing setup before I had a good idea this would work, so I only added one new build script using firefox-mobile.

The npm run test won't work (or actually it might, but would just build the old way, I guess so TravisCI blows up), because I didn't make any changes to lib/helper.js. However, running the new script does appear to output all the files and manifest.json looks OK.

I can continue on -- fixing the tests, deleting the old configs, etc. -- if we decide to move forward.

r? @miketaylr

miketaylr commented 6 years ago

This approach seems reasonable to me, @laghee. Go for it!

laghee commented 6 years ago

OK, ESLint is making me mental.

Apparently the --fix flag won't work on indent errors, so I tried to fix all these by hand. But I must be doing something goofy because if I shift every line to the left by 2 spaces ... it gives me the opposite error and says everything needs to be shifted right by 2 spaces. 😱 (Initially I had identical errors in both files that are failing:

screenshot 2018-05-30 16 07 08

... but after attempting to fix the top file, I get the Travis detail view.)

What does it want from me?? Maybe Cloud9 has some weird text encoding thing like Mac TextEdit where it encodes extra stuff in the spaces? ~I'll check.~ Nope. That's not it.

I'm not 100% clear on how ESLint and Prettier work together, but is it possible that they're in conflict over how to format a switch statement? Those are the only lines affected, and the error feedback is different in the Travis details between the top file and the bottom one. 🤔

**I think this^ might be it. ESLint is defaulting to no indent on cases inside switch statements, while Prettier indents by 2. ESLint doesn't normally take precedence over Prettier, but we have the indent rule enabled, which defaults to 0 on switch cases.

miketaylr commented 6 years ago

(note: you've got a typo in your commit message -- "weebpack config file")

I'm not 100% clear on how ESLint and Prettier work together, but is it possible that they're in conflict over how to format a switch statement? Those are the only lines affected, and the error feedback is different in the Travis details between the top file and the bottom one.

Hmmm, that might be an issue. So far I haven't had any conflicts with the current setup... but maybe https://github.com/prettier/prettier-eslint is a better integration option? It mentions

ESLint plugin. While prettier-eslint uses eslint --fix to change the output of prettier, eslint-plugin-prettier keeps the prettier output as-is and integrates it with the regular ESLint workflow.

But prettier should be doing the indentation for you, you shouldn't have to format anything by hand. 😕

laghee commented 6 years ago

Hmmm, that might be an issue. So far I haven't had any conflicts with the current setup... but maybe https://github.com/prettier/prettier-eslint is a better integration option?

Might be worth looking at. This particular case might be solved with a tweak to the eslint indent rule. The .eslintrc has "indent": [2, 2], -- I can't figure out what the first two is, but since SwitchCase property isn't specified, I think it's defaulting to zero.

I'll look at both.

(note: you've got a typo in your commit message -- "weebpack config file")

Aaaarrgh. The terminal display in Visual Studio does weird things when I try to backspace, and I can't tell where the cursor is. 🙈

miketaylr commented 6 years ago

This particular case might be solved with a tweak to the eslint indent rule. The .eslintrc has "indent": [2, 2],

sweet -- if it's easier to just add that to the .eslintrc, go for it.

Aaaarrgh. The terminal display in Visual Studio does weird things when I try to backspace, and I can't tell where the cursor is.

yikes, doesn't sound fun!

laghee commented 6 years ago

\o/ I tweaked the .eslintrc, but it looks like the prettier-eslint option could be a good alternative for a future change.

I've noticed before that a lot of spacing issues get double errors (one from each), which seems like extraneous yelling. Plus, I couldn't get eslint --fix to actually fix anything this time. It seemed to think it was running and making changes ... but the file stayed exactly the same despite the fixer making 10 passes according to the debugger 🤷‍♀️.

miketaylr commented 6 years ago

Btw, nice commits and style!