Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Prelint plugins #105

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Prelint plugins #105

wants to merge 3 commits into from

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Feb 6, 2023

Summary

This RFC introduces the concept of a "prelint", which is like a rule but it runs before any linting happens, allowing plugins to modify the behavior of subsequent linting runs.

This requires #99 to function and was split off from that RFC.

Related Issues

#99
eslint/eslint#14745

@ljharb
Copy link

ljharb commented Feb 6, 2023

Does this include a way for a rule to declare a dependency on a prelint, and implicitly "add" it to the prelint list if not already present?

@nzakas
Copy link
Member Author

nzakas commented Feb 6, 2023

@ljharb no. Rules cannot force users to add other rules, prelints, or configurations. At least to start, rules should specify what they’re expecting in their docs. Plug-ins can, of course, publish configs that contain appropriate “known good” settings.

@nzakas nzakas added Initial Commenting This RFC is in the initial feedback stage and removed triage labels Feb 6, 2023

context.createVirtualFile({
filename: "0.js",
body: removeIndents(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly off-topic - I would envision that most tools would want to strip indent in the same way - might be good to provide a util from eslint itself that does this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly worth thinking about -- also worth thinking about: does ESLint just do it automatically for you based on the indentOffset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the proposal to automatically adjust indents.

@ljharb
Copy link

ljharb commented Feb 7, 2023

Gotcha - that's fine, it just means this doesn't solve the use case of allowing rules to execute a "before" step.

@nzakas
Copy link
Member Author

nzakas commented Feb 7, 2023

@ljharb is there a specific use case you have in mind?

@ljharb
Copy link

ljharb commented Feb 7, 2023

Yes, I want eslint-plugin-import's rules to be able to indicate that they need an "initialization" step, to cache the module graph, and for eslint to automatically queue up the Promise ordering as appropriate, and to skip the initialization step when no active rules require it.

@nzakas
Copy link
Member Author

nzakas commented Feb 10, 2023

Ah okay, thanks for the explanation. I'll need to think about that use case a bit more, but I don't think it fits within this RFC.

Comment on lines 201 to 211
/**
* The number to add to any violations that occur in the first line of
* the virtual file. 0 if undefined.
*/
columnStart?: number;

/**
* The number to add to the column number of each violation in the body.
* 0 if undefined.
*/
indentOffset?: number;
Copy link
Member

@ota-meshi ota-meshi Feb 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to tell the ESLint engine the negative column offset of a line of text?

For example, if I have HTML like the following (for a 2-space indent), if I want the CSS in the style attribute to have an indentOffset of 4, the font-size will be placed at a column offset of -4.

<div>
  <div style="
    color: red;
font-size: 16px;
  ">

Copy link
Member

@ota-meshi ota-meshi Feb 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I want to use 4 for indentOffset is because I want to convey the base position for the indent rule. If it's 0, I don't think the indent rule work well. It's probably indented like this:

<div>
  <div style="
color: red;
font-size: 16px;
  ">

I think people actually want indent like this:

<div>
  <div style="
    color: red;
    font-size: 16px;
  ">

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to tell the ESLint engine the negative column offset of a line of text?

Hmmm, I'm not sure I understand why you'd want to do this. If you have this HTML:

<div>
  <div style="
    color: red;
font-size: 16px;
  ">

And you want to extract the CSS, for linting, I'd think you'd want ESLint to see the CSS as this:

    color: red;
font-size: 16px;

So you'd say:

context.createVirtualFile({
    body: removeIndents(cssNode.text),    // get the text you want somehow
    lineStart: 3,    // add this number to any reported lines
    columnStart: 0,    // add this number to any columns reported in the first line
    indentOffset: 0    // don't adjust indents
});

So then you'd take the CSS virtual file, normalize the indents, then that would get merged back into the HTML.

Would that work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think I didn't explain well 😓. (Prelint may be an unrelated topic.)
How do you think indentation rules for CSS should be implemented to handle indentation correctly given text like this?

<div>
  <div style="
    color: red;
font-size: 16px;
  ">

I would expect it to be indented like this:

<div>
  <div style="
    color: red;
    font-size: 16px;
  ">

However, I think the current system would probably indent like this:

<div>
  <div style="
color: red;
font-size: 16px;
  ">

Because the CSS indentation rules don't know the proper starting position.

I thought it could be solved by indenting based on 0 if a negative column offset could be used, but there may be another way to solve it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you mean. Indeed, the current design doesn't allow for that. I need to think a bit more about how to handle this.

I think negative indents would be a bit confusing, as a negative indent would mean "add indents" while a positive indent would mean "remove indents".

It might be the case that we need some sort of callback function to allow prelints to further modify a fixed result before inserting into the original file.

I'll think more on this and see what I can come up with.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so it turns out this will work, just not in the most obvious way.

First, we actually need two different rules, one for CSS and one for HTML. Let's call them css/indent to indent CSS and html/indent to indent HTML.

So to start, we have this code:

<div>
  <div style="
    color: red;
font-size: 16px;
  ">

The prelint would extract the CSS into a text fragment that looks like this:

    color: red;
font-size: 16px;

The css/indent rule would fix the indentation to this:

color: red;
font-size: 16px;

Then, this CSS would be inserted back into the HTML, looking like this:

<div>
  <div style="
color: red;
font-size: 16px;
  ">

When the html/indent rule then runs, it would end up fixing to this:

<div>
  <div style="
    color: red;
    font-size: 16px;
  ">

Basically, we'd be combining the fixes from css/indent and html/indent together either in one pass (if technically possible) or at most in two passes (ESLint does multiple passes when attempting to autofix, so this is not new).

@nzakas
Copy link
Member Author

nzakas commented Feb 13, 2023

@JamesHenry this RFC might be of interest to you, as well.

@JamesHenry
Copy link
Member

JamesHenry commented Feb 16, 2023

Thanks @nzakas I can comment on the big 3 projects I work on:

Based on my understanding, I think it would just give an alternative to the use of processors for angular-eslint - I don't think it has any major advantages of the current implementation (but also no major disadvantages)

For typescript-eslint and Nx, I don't think this would change much, I think for those two we would need something much more like what @ljharb is describing

@nzakas
Copy link
Member Author

nzakas commented Feb 16, 2023

@JamesHenry thanks for the feedback. Indeed, I'm viewing this as a more flexible replacement for processors rather than something radically different. The primary different is that it would allow us to lint both the original file and all of the virtual files, whereas processors only allow linting of virtual files.

@nzakas
Copy link
Member Author

nzakas commented Feb 20, 2023

Updated to simplify the creation of fragments out of a file. You no longer need to manually manipulate the text before passing it back to ESLint. Let me know what you think.

@eslint/eslint-tsc still waiting for the first round of feedback on this.

}
```

Each prelint follows the same basic structure of as a rule, only it cannot report violations and instead can only inspect the AST, use `SourceCode` to make any necessary changes, and create fragments as necessary. Otherwise, it looks the same as a rule, such as:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we mean by "use SourceCode to make any necessary changes", what kind of changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be language-dependent. In JavaScript, we have markVariableAsUsed() as an example.

Comment on lines +100 to +103
// prelint with options
"markdown/js": {
codeBlockTags: ["js", "javascript", "ecmascript"]
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when multiple config entries that specify options for markdown/js match the given file, are the options objects merged or does the last one overwrite previous ones?

Alternatively, since prelints have access to configured settings (PrelintContext.settings), maybe we don't need to support prelint options because they can be specified inside settings for the plugin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options are merged in the same way that rule options are merged.

settings has always been a hack and I wouldn't want to lean on it any more.


In order to make all of this work, we'll need to make the following changes in the core:

1. `linter.js` will need to be updated to treat fragments the same as processor code blocks (recursively linting) and to do a first traversal of the file using prelints.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linter.js also needs to be updated to postprocess lint messages from fragments? In particular, to calculate locations and fix ranges.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! I'm going to circle back and do some more details around the implementation once I finish up the languages RFC.

@brettz9
Copy link

brettz9 commented Mar 20, 2023

Hi,

Just wanted to confirm--would this support multiple levels of nesting with languages?

My use case would be for @example tags containing JavaScript to lint within a JSDoc block "language" (so that the blocks could have their own AST which rules could target), within the main JavaScript file.

So after parsing the main JavaScript AST, we'd want to find the /** */ blocks and parse their contents as our own JSDoc AST, and then when finding @example within the latter, parse its inner contents again as JavaScript AST.

@nzakas
Copy link
Member Author

nzakas commented Mar 27, 2023

Yes, based on your config it could go multiple levels deep. So if you have a Markdown file that has a JavaScript block, that would get pulled out, and then any prelints configured for JavaScript files would run. So, if the JavaScript block contained a block of GraphQL, a prelint could pull that out as separate from the Markdown and JavaScript that it was embedded within.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants