Bug fixes #60
Bug fixes #60
Conversation
Changing filenames in the return value of `preprocess()` automatically triggers the processor again for those "new files". This adds a guard to bail in processing if those temporary files come in for new processing.
khawkins
requested review from
dbreese,
sfdctaka,
maliroteh-sf and
a team
as code owners
| { | ||
| files: ['**/*.html'] | ||
| } | ||
| ] |
There was a problem hiding this comment.
When html overrides is removed, will any rule be applied to html files?
| } | ||
| overrides: [ | ||
| { | ||
| files: ['*.html', '**/*.html', '*.js', '**/*.js'], |
There was a problem hiding this comment.
Ok, I see the html extension here.
| const bundleKey = extractBundleKey(filename); | ||
| if (bundleStateManager.hasBundleWithKey(bundleKey)) { | ||
| // Skip processing for files we generated | ||
| return [{ text, filename }]; |
There was a problem hiding this comment.
Does this fix recursive processing?
There was a problem hiding this comment.
Yes. The renaming we do at the bottom of the method causes ESLint to (re-)process this "new" file that we created. Conversely, when you return what you got in the processor, ESLint continues on with the rest of the process (i.e. considers that file pre-processed).
| /** | ||
| * Concrete implementation of StaticAnalyzerInterface that uses the actual static analyzer | ||
| */ | ||
| class StaticAnalyzerProvider extends StaticAnalyzerInterface { |
There was a problem hiding this comment.
I pulled this out into its own interface implementation so we could test the outputs coming from the static analysis.
| // Only include diagnostics that match both the rule and target the primary file | ||
| return ( | ||
| reportDiagnosticValue === diagnosticValue && | ||
| reportDiagnostic.code.target.path === lwcBundle.primaryFile.filename |
There was a problem hiding this comment.
I started seeing a random squiggly line for an HTML violation, in the middle of my JS file. 😂
| /** | ||
| * Interface for static analysis functionality | ||
| */ | ||
| class StaticAnalyzerInterface { |
There was a problem hiding this comment.
Is this a mirror from Komaci library?
There was a problem hiding this comment.
Not per se. It was just a way to abstract the Komaci analysis functions we use, so that I could create a mock provider in test, to test how the analysis output impacted different parts of the reporting. You can see some of the mocked analysis tests below.
| // Set the primary file before getting the bundle key | ||
| bundle.setPrimaryFileByContent(jsContent); | ||
| const bundleKey = bundle.getBundleKey(); | ||
| bundleStateManager.addBundleState(bundle); |
There was a problem hiding this comment.
Is this line necessary since bundleAnalyzer.preprocess can handle that?
There was a problem hiding this comment.
It could in theory, i.e. if you called bundleAnalyzer.preprocess() once with the given bundle, then called it again below. This is just meant to simulate that process by staging the bundle in the state manager, the gating factor for whether it has been processed already. One way or another you'd have to call something, since this test is meant to cover what happens when the same file goes through preprocessing again.
What does this PR do?
This fixes a couple of issues seen in the wild.
preprocess()method, it recursively runs that "new" file through the processor again. This was causing weird inconsistencies and infinite loops, because thepreprocess()flow was not expecting these files to come in. There's now a guard to fall through for these files.eslintReports.