refactor: Extract processor logic into ProcessorService

[nzakas]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

This extracts processor logic into ProcessService:

• Created ProcessorService
• Refactored Linter to use ProcessorService
• Added rawBody to VFile
• Updated VFile tests

The biggest difference is that ProcessorService takes a VFile and returns an array of VFiles (or strings for legacy compat). This makes it easier to pass VFile instances around without worrying if it's from a processor or not.

Is there anything you'd like reviewers to focus on?

• I'm unsure if the parameter order for ProcessorService#postprocessSync() makes sense. I like the idea of always having config as the last parameter, but would like some other opinions.
• I think we can avoid adding rawBody to VFile if we always pass BOM-less file contents to a processor. See inline comment.

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	b617ad7
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/66eaf99fd8107800097a3b7d

[nzakas]

Created this to avoid creating a second VFile for each block.

[nzakas]

Same here. Wanted to avoid creating two VFiles for the same block.

[nzakas]

Right now, we actually pass the raw file contents to processors, which includes the BOM if present. I added this to make existing tests pass, however, I think it would be better to always strip the BOM before passing to a processor.

This wouldn't be a breaking change because processors must always check for the presence of BOMs right now as they don't know if one will be passed.

[mdjermanovic]

I think it's safer to keep passing the raw file contents to processors because preprocess() can return the text as-is, in which case the file will be linted as if there was no processor.

eslint/lib/linter/linter.js

Lines 1592 to 1607 in 156b1c3

	// Resolve configuration again if the file content or extension was changed.
	if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) {
	debug("Resolving configuration again because the file content or extension was changed.");
	return this._verifyWithFlatConfigArray(
	blockText,
	configForRecursive,
	{ ...options, filename: blockName, physicalFilename }
	);
	}
	// Does lint.
	return this._verifyWithFlatConfigArrayAndWithoutProcessors(
	blockText,
	config,
	{ ...options, filename: blockName, physicalFilename }
	);

On the other hand, not passing BOM would fix bugs in processors that do not check if the BOM is present, which happens to be the case with @eslint/markdown (autofixes are off-by-one in files with BOM).

[mdjermanovic]

This file was probably accidentally left here after testing #18742 (comment)

[mdjermanovic]

Related to #18818 (comment)

Before this change, with this config file:

// eslint.config.js
module.exports = [{
    files: ["**/*.myjs"],
    processor: {
        preprocess(text, filename) {
            return [{ text, filename }];
        },
        postprocess(messages) {
            return [].concat(...messages);
        },
        supportsAutofix: true
    },
    rules: {
        "unicode-bom": ["error", "never"]
    }
}];

and this test file (has BOM):

// test.myjs

var x = "This file has BOM";

Running eslint test.myjs would report that the file has BOM, and running eslint test.myjs --fix would remove it.

After this change, eslint test.myjs doesn't report that the file has BOM.

[nzakas]

Oops yeah, that would have to be rawBody to make this work.

[nzakas]

It seems like processors not checking for BOMs is a likely source of bugs, so I'm more in favor of passing BOM-less text to processors now. What do you think?

[nzakas]

I've updated the PR so that BOMs are not passed to processors any more. If we decide not to move forward with that, I can revert that commit, but I do think this is the right choice based on previous discussion.

[mdjermanovic]

I've updated the PR so that BOMs are not passed to processors any more.

Makes sense to me. It seems unlikely that a processor would particularly want to know if the original file has BOM.

However, I think we should ensure that, when the processor returns a block with the entire content that was passed to it (in order to lint the file as a whole + its code blocks), the original file with BOM is linted. The example in #18818 (comment) still doesn't work with the latest version of this branch.

[nzakas]

However, I think we should ensure that, when the processor returns a block with the entire content that was passed to it (in order to lint the file as a whole + its code blocks), the original file with BOM is linted. The example in #18818 (comment) still doesn't work with the latest version of this branch.

I think the only way to achieve this would be to add a third parameter to preprocess that indicates if there's a BOM. That way, processors could decide whether or not they wanted to pass the BOM along in their code blocks. That preserves the benefits of stripping the BOM from the text so processors don't forget to check while allowing processors to still opt-in to checking for it if they so desire.

(I think in an ideal world, processors would accept and return VFile objects, which would carry that additional information. But this would mean a v2 processors API just to support this use case, and I'm not sure it's worth that much effort.)

[nzakas]

Updated to preserve current behavior.

Instead of adding rawBody to VFile (which always felt a little gross), I'm just recalculating the correct body when it's necessary.

[nzakas]

The failure is just the pull request labeler. No idea what's going on with that.

[sam3k]

Per 2024-09-05 TSC meeting, we will preserve the current behavior and @nzakas will open an issue to discuss changing it in v10

[mdjermanovic]

Sorry for the delay, I'll review this soon.

[mdjermanovic]

The example from #18818 (comment) still doesn't work, because text !== block.body is true when file has BOM so it enters this code path and lints block.body which doesn't include BOM.

[nzakas]

I went back to the original implementation that preserve the existing BOM behavior and added a test for Linter that validates the BOM is being passed.

[mdjermanovic]

LGTM, thanks!