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: skip processing code blocks on specific languages like stylelint-prettier #415

Merged
merged 10 commits into from
Jun 27, 2022
Merged

feat: skip processing code blocks on specific languages like stylelint-prettier #415

merged 10 commits into from
Jun 27, 2022

Conversation

JounQin
Copy link
Member

@JounQin JounQin commented Apr 22, 2021

close #412, #414

Just like prettier/stylelint-prettier#22

The e2e test is a bit difficult to be added because we're supporting very long time ago versions of ESLint/Node, and such e2e test cases requires a lot of ESLint plugins to be installed which are very likely incompatible with old versions of ESLint/Node.

If you have a good idea to add e2e tests, I'll follow your advices.

@JounQin
Copy link
Member Author

JounQin commented May 12, 2021

@BPScott Friendly ping.

@BPScott
Copy link
Member

BPScott commented May 25, 2021

Heya @JounQin, sorry for the long delay.

This change and #413 both touch the same portion of code. Is the expectation that only one of them should be merged? If so which PR is preferable? I'm guessing this one as it was creating slightly later?

I was originally a bit confused by the title of this PR as we already have logic in place to skip processing code blocks when encountering languages that are not JS. But I think I understand what is happening - you're adding a new case that says we should bail out of doing any processing when both filepath !== onDiskFilepath (aka this file has been processed by a processor that provides a virtual filename) AND when the inferred parser is one of the following: [ 'babel', 'babylon' 'flow' 'typescript', 'vue', 'markdown', 'mdx' 'html'].

I think I'd have found this change a bit easier to understand if instead of adding an extra nested condition to existing code, the following block was added above line 209 (leaving the existing code unchanged):

const nonVirtualProcessorParserBlockList = ['babel', 'babylon' 'flow', 'typescript',  'vue', 'markdown', 'mdx' 'html'];
if (
  filepath !== onDiskFilepath &&
  nonVirtualProcessorParserBlockList .indexOf(prettierFileInfo.inferredParser) !== -1) {
  return;
}

Could you explain why the that list of parsers was chosen for when you're not dealing with a file created by a processor that specifies a virtual filename? I would have expected the list to be the same as when you're using virtual filenames?


I've been thinking for a while it might soon be time to drop older node / eslint / prettier versions. It seems like eslint/eslint#14616 is close to being merged. Perhaps I should wait till a version of eslint is released with that change present, and that can be eslint-plugin-prettier v4. That'd massively simplify a lot of the "filename on disk" logic.

@JounQin
Copy link
Member Author

JounQin commented May 25, 2021

@BPScott Thanks for reply finally first.

This change and #413 both touch the same portion of code. Is the expectation that only one of them should be merged? If so which PR is preferable? I'm guessing this one as it was creating slightly later?

They are both valid, while #413 only add support for @graphql-eslint, and this PR doesn't touch that. If you want, I can merge them, but I prefer to separate the two PRs personally because they are different.

Could you explain why the that list of parsers was chosen for when you're not dealing with a file created by a processor that specifies a virtual filename? I would have expected the list to be the same as when you're using virtual filenames?

I just want to keep compatibility and borrowed the list from stylelint-prettier. Maybe the list can be updated. 🤣

The original list is used for some legacy ESLint plugins as I commented:

// The following list means the plugin process source into js content
// but with same filename, so we need to change the parser to `babel`
// by default.

// Related ESLint plugins are:
// 1. `eslint-plugin-graphql` (replacement: `@graphql-eslint/eslint-plugin`)
// 2. `eslint-plugin-markdown@1` (replacement: `eslint-plugin-markdown@2+`)
// 3. `eslint-plugin-html`

@JounQin
Copy link
Member Author

JounQin commented May 25, 2021

I've been thinking for a while it might soon be time to drop older node / eslint / prettier versions. It seems like eslint/eslint#14616 is close to being merged. Perhaps I should wait till a version of eslint is released with that change present, and that can be eslint-plugin-prettier v4. That'd massively simplify a lot of the "filename on disk" logic.

That would just help to remove getOnDiskFilepath inside, nothing else, doesn't it? I think it is good to support old version of ESLint if it just requires a simple function. But of course, dropping older node/eslint/prettier versions are great too. I'm fine with that. v4 for BREAKING CHANGE like this PR is good.

@BPScott
Copy link
Member

BPScott commented May 25, 2021

They are both valid, while #413 only add support for @graphql-eslint, and this PR doesn't touch that. If you want, I can merge them, but I prefer to separate the two PRs personally because they are different.

OK, that makes sense - I see that @graphql-eslint handles graphql files but uses virtual filenames (where as eslint-plugin-graphql does not use virtual filenames)

The original list is used for some legacy ESLint plugins as I commented:

I don't think legacy is the right word here. It is totally valid for eslint plugins to choose not to use virtual filenames. I'd absolutely prefer that they did use virtual filenames, but I don't think there's anything that suggests not specifying a filename going to be unsupported by eslint in the future.

I just want to keep compatibility and borrowed the list from stylelint-prettier. Maybe the list can be updated. 🤣

I think it will need to be updated. THe list in stylelint-prettier is designed to removed non-css file types but for eslint-plugin-prettier we'd want to remove non-js file types - for instance currently this code does an early return when the parser is babel which means all JS files will be skipped! I thiiiiink it ends up being the same list as the current parser block list, but I'll need to think about that a bit more.

That would just help to remove getOnDiskFilepath inside, nothing else, doesn't it?

There are a few other simplifications too. e.g. supporting only prettier v2 means the supportBabelParser check can be removed as that will be always true (it is only false in prettier <1.16). It will also make writing e2e tests easier as we won't have to handle all those old versions as you mentioned in your original post.

@JounQin
Copy link
Member Author

JounQin commented May 25, 2021

I don't think legacy is the right word here. It is totally valid for eslint plugins to choose not to use virtual filenames. I'd absolutely prefer that they did use virtual filenames, but I don't think there's anything that suggests not specifying a filename going to be unsupported by eslint in the future.

Oh, sorry, but I mean: eslint-plugin-graphql should be replaced by @graphql-eslint, eslint-plugin-markdown@1 has been upgraded to eslint-plugin-markdown@2+ which uses virtual filenames, and eslint-plugin-html wants to do like so too: BenoitZugmeyer/eslint-plugin-html#139.

for instance currently this code does an early return when the parser is babel which means all JS files will be skipped

That's not true, they are just skipped when *-in-js, for example, graphql-in-js like #413 (comment).

The original js/ts file will still be processed because filepath === onDiskFilepath. That's why we should treat virtual filename and physical filename separately.

There are a few other simplifications too. e.g. supporting only prettier v2 means the supportBabelParser check can be removed as that will be always true (it is only false in prettier <1.16). It will also make writing e2e tests easier as we won't have to handle all those old versions as you mentioned in your original post.

Right, I was saying your mentioned ESLint PR only adds getPhysicalFilename method, that does not make many senses. But for e2e tests, I'm glad to upgrade all versions.

@BPScott
Copy link
Member

BPScott commented May 31, 2021

That's not true, they are just skipped when *-in-js, for example, graphql-in-js like #413 (comment).

I don't think processors needed to do anything for *-in-js as you've described - when given a JS file ESLint doesn't need to run a processor over it - you don't need to transform file content into valid JS if it is already JS! Processors only kick in when you're transforming a non-js file into valid JS. e.g. turning a .graphql into JS by wrapping it in a tagged template literal, or extracting fenced code blocks out of an .md file.

These processors are only ever going to create filenames like myfile.graphql/foo.js

I agree with the sentiment in #413 (comment) and prettier/stylelint-prettier#22 that eslint-plugin-prettier shouldn't bother running prettier over these fragments of a file: If you're this interested in prettier then you probably want to run prettier over the rest your .graphql / .mdx etc file to to get it fully formatted. Thus it seems pointless to duplicate that work of running prettier within eslint.

@JounQin
Copy link
Member Author

JounQin commented May 31, 2021

I don't think processors needed to do anything for *-in-js as you've described - when given a JS file ESLint doesn't need to run a processor over it - you don't need to transform file content into valid JS if it is already JS! Processors only kick in when you're transforming a non-js file into valid JS. e.g. turning a .graphql into JS by wrapping it in a tagged template literal, or extracting fenced code blocks out of an .md file.

@graphql-eslint is also processing .js files for gql tags. What means:

// test.js

const test = gql`
  # content
`

See also: https://github.com/dotansimha/graphql-eslint#configuration

And also @angular-elsint lints templates in .component.ts too.

See also https://github.com/angular-eslint/angular-eslint/blob/master/packages/eslint-plugin-template/src/processors.ts#L259

Copy link

@rtrembecky rtrembecky left a comment

Choose a reason for hiding this comment

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

just noticed a typo so bringing it up 😅

eslint-plugin-prettier.js Outdated Show resolved Hide resolved
@BPScott
Copy link
Member

BPScott commented May 31, 2021

I don't think processors needed to do anything for *-in-js as you've described - when given a JS file ESLint doesn't need to run a processor over it - you don't need to transform file content into valid JS if it is already JS! Processors only kick in when you're transforming a non-js file into valid JS. e.g. turning a .graphql into JS by wrapping it in a tagged template literal, or extracting fenced code blocks out of an .md file.

@graphql-eslint is also processing .js files for gql tags. What means:

// test.js

const test = gql`
  # content
`

See also: https://github.com/dotansimha/graphql-eslint#configuration

And also @angular-elsint lints templates in .component.ts too.

See also https://github.com/angular-eslint/angular-eslint/blob/master/packages/eslint-plugin-template/src/processors.ts#L259

Yep sure but what's the result of context.getFilename(); in these cases? I would expect that to be the filename that matches the filename on disk that is already a JS file - test.js rather than some virtual filename test.js/_0.graphql/0.js

@JounQin
Copy link
Member Author

JounQin commented Jun 1, 2021

Yep sure but what's the result of context.getFilename(); in these cases? I would expect that to be the filename that matches the filename on disk that is already a JS file - test.js rather than some virtual filename test.js/_0.graphql/0.js

For @graphql-eslint, it's test.js/0_document.graphql(https://github.com/dotansimha/graphql-eslint/blob/master/packages/plugin/src/processors/code-files.ts#L36) and it has a .graphql parser: https://github.com/dotansimha/graphql-eslint/blob/master/packages/plugin/src/parser.ts#L16

For @angular-eslint, it's test.ts/0_inline-template-0.component.html(https://github.com/angular-eslint/angular-eslint/blob/master/packages/eslint-plugin-template/src/processors.ts#L221) and it has a .html parser: https://github.com/angular-eslint/angular-eslint/blob/master/packages/template-parser/src/index.ts#L179

@JounQin
Copy link
Member Author

JounQin commented Jun 25, 2021

@BPScott Any news? It's already two months after our last conversation...

@BPScott
Copy link
Member

BPScott commented Aug 20, 2021

Heya @JounQin. Thank you for your patience. As you've saw I've found it hard to find time for this project thanks to personal stuff.

I wanted to spend some time understanding how processors interact with tests and if getFilename stuff made any of that easier, I managed a bit but didn't find any easy answers. I see you've pushed for a bunch of improvements in eslint/eslint#14616 and eslint/eslint#14800. Thank you for doing all of that, hopefully eslint adds eslint/rfcs#31 at some point to make the e2e testing of all this a lot easier.

I've merged #413 and have released it as v3.4.1. Would you be able to rebase this PR atop latest master. When you originally opened this PR i wasn't sure how to reconcile the the conflicting changes in #413 and this PR.

I'll try and drop the old versions of eslint in a major version at some point, which should let us leverage getPhysicalFilename() and simplify some of this.

@JounQin
Copy link
Member Author

JounQin commented Aug 21, 2021

Thanks @BPScott

No worry, I've already had a rebased branch locally, I'll push it when I'm free today.

Before eslint/rfcs#31, we can still use our own physicalFilename, or only use it on testing for workaround.

@JounQin
Copy link
Member Author

JounQin commented Aug 21, 2021

@BPScott Already rebased!

@JounQin
Copy link
Member Author

JounQin commented Sep 7, 2021

@BPScott I just added ESLint API based fixtures support at https://github.com/prettier/eslint-plugin-prettier/pull/415/files#diff-79377e9ab524ea5107fe58032bf72dae0a8eee2c210275404d346a98908b2b78R258-R297

Maybe snapshot based test cases would be better.

@JounQin
Copy link
Member Author

JounQin commented Oct 14, 2021

@BPScott Any news again?

@dimaMachina
Copy link

@BPScott any news? 🙏

@shaneog
Copy link

shaneog commented Feb 19, 2022

I just ran into this issue.

@BPScott Is there any issue with proceeding with this?
@JounQin - would you be able to fix the conflicts if @BPScott has no issues?

@JounQin
Copy link
Member Author

JounQin commented Jun 26, 2022

Rebased.

It becomes a bit ridiculous since the PR has been raised for more than one year!

I'll have to publish a fork instead later...

@JounQin
Copy link
Member Author

JounQin commented Jun 26, 2022

Just released @rxts/eslint-plugin-prettier@4.1.0, source codes at https://github.com/JounQin/eslint-plugin-prettier/tree/fork-release, you can use

# yarn
yarn add -D eslint-plugin-prettier@npm:@rxts/eslint-plugin-prettier@^4.1.0

# pnpm
pnpm add -D eslint-plugin-prettier@npm:@rxts/eslint-plugin-prettier@^4.1.0

# npm
npm i -D eslint-plugin-prettier@npm:@rxts/eslint-plugin-prettier@^4.1.0

Then it will just work the same as before with this feature enabled.

@dimaMachina
Copy link

@BPScott please merge this PR

@JounQin JounQin merged commit 52eec48 into prettier:master Jun 27, 2022
@JounQin JounQin deleted the fix/code_blocks branch June 27, 2022 12:25
@JounQin
Copy link
Member Author

JounQin commented Jun 27, 2022

https://github.com/prettier/eslint-plugin-prettier/releases/tag/v4.1.0

eslint-plugin-prettier@v4.1.0 has been released!

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 28, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint-plugin-prettier](https://github.com/prettier/eslint-plugin-prettier) | devDependencies | minor | [`4.0.0` -> `4.1.0`](https://renovatebot.com/diffs/npm/eslint-plugin-prettier/4.0.0/4.1.0) |

---

### Release Notes

<details>
<summary>prettier/eslint-plugin-prettier</summary>

### [`v4.1.0`](https://github.com/prettier/eslint-plugin-prettier/blob/HEAD/CHANGELOG.md#v410-2022-06-27)

[Compare Source](prettier/eslint-plugin-prettier@v4.0.0...v4.1.0)

-   feat: skip processing code blocks on specific languages like `stylelint-prettier` ([#&#8203;415](prettier/eslint-plugin-prettier#415)) ([52eec48](prettier/eslint-plugin-prettier@52eec48))
-   build(deps): Bump minimist from 1.2.5 to 1.2.6 ([#&#8203;464](prettier/eslint-plugin-prettier#464)) ([42bfe88](prettier/eslint-plugin-prettier@42bfe88))
-   build(deps-dev): Bump graphql from 15.5.1 to 15.7.2 ([#&#8203;442](prettier/eslint-plugin-prettier#442)) ([0158640](prettier/eslint-plugin-prettier@0158640))
-   build(deps-dev): Bump [@&#8203;graphql-eslint/eslint-plugin](https://github.com/graphql-eslint/eslint-plugin) from 2.3.0 to 2.4.0 ([#&#8203;444](prettier/eslint-plugin-prettier#444)) ([4bcaca2](prettier/eslint-plugin-prettier@4bcaca2))
-   chore(CI): add tests for ESLint 8 ([#&#8203;428](prettier/eslint-plugin-prettier#428)) ([f3713be](prettier/eslint-plugin-prettier@f3713be))
-   README.md: HTTP => HTTPS ([#&#8203;443](prettier/eslint-plugin-prettier#443)) ([44e1478](prettier/eslint-plugin-prettier@44e1478))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [x] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1435
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incompatible with markdown files and eslint-plugin-mdx out of box
5 participants