feat(gatsby): add required eslint rules even if user has custom eslint config #28911
Conversation
gatsbot
bot
added
the
status: triage needed
pieh
added
topic: hot reloading*
and removed
status: triage needed
pieh
force-pushed
the
fast-refresh/merge-eslint-rules-with-custom-ones
branch
from
5c4170d
to
479af2c
Compare
| if (existingOptions.baseConfig.extends) { | ||
| if ( | ||
| Array.isArray(existingOptions.baseConfig.extends) && |
There was a problem hiding this comment.
So if .extends exists but is not an array then the eslintRequirePreset part is silently dropped? Smells like a bit like a footgun.
There was a problem hiding this comment.
You are right, I thought extends only accept arrays but it also accept just strings, so I will add handling for it
There was a problem hiding this comment.
Added handling for single string case in f5ad45a + some tests for it
| // and adjust it to add the rule or append new loader with required rule | ||
| const rule = config.module.rules.find(rule => { | ||
| if (typeof rule.loader === `string`) { | ||
| return rule.loader.includes(`eslint-loader`) |
There was a problem hiding this comment.
Includes? No risk of false positives here?
There was a problem hiding this comment.
maybe I can keep includes('eslint-loader') as "precheck" and if those are satisfied do a deeper check (i.e. read package.json of imported package and see if package actually is eslint-loader), but no matter how I slice it - it will be hacky :(
There was a problem hiding this comment.
I think filename based validation is fine here. But a substr check seems a little too much of a shortcut, that's all.
There was a problem hiding this comment.
Hmmm, so do you have any advice on what would good to do here?
I can cover maybe few specific scenario:
loaderis defined by package name (not fully resolved) - so for this I would do strict comparison toeslint-loader(no includes)loaderhas some resolved path (so eithereslint-loader/index.jsoreslint-loader/dist/cjs.js) - then I can do includes or regex matching those
There was a problem hiding this comment.
Added hopefully more strict checks in f477549 which I don't particularly like (but neither I did includes('eslint-loader')) as it's hacky no matter how I slice it
Description
Change building on #28689 (so targeting that branch and not
master.This should ensure fast-refresh related rules are included even if user have custom config or disable set of builtin rules. Those required rules are warnings only so they will not block building webpack bundle.
Note - this was tested when adding local
.eslintrcfile in 2 variants: without usinggatsby-plugin-eslintand using it. It likely might not work wheneslint-loaderis added to webpack differently (as in it will add second instance ofeslint-loader), but given thatgatsby-plugin-eslintlooks like "go-to" approach, it might be fine to leave as is[ch22490]