Shinigami92 / eslint-plugin-vue-pug-sfc

eslint plugin vue pug sfc
https://www.npmjs.com/package/eslint-plugin-vue-pug-sfc
MIT License
32 stars 4 forks source link

Migrate `script-setup-uses-vars` #158

Closed rashfael closed 2 years ago

rashfael commented 2 years ago

Reference https://github.com/Shinigami92/eslint-plugin-vue-pug-sfc/issues/8

Continuing from https://github.com/Shinigami92/eslint-plugin-vue-pug-sfc/pull/155

Mark as used:

rashfael commented 2 years ago

I encountered a few issues which I'd like feedback on before I start to change code:

  1. This rule needs to be called on Programm instead of Programm:exit because it influences another existing rule. This is currently just hacked in here https://github.com/Shinigami92/eslint-plugin-vue-pug-sfc/pull/158/files#diff-6147e3c1761df71fd40952dbcbac388a45f80b8c318f367ef41048351a2c0c99R106 and obviously needs to be fixed properly. We could add an option to processRule. vue-eslint-parser has an templateBodyTriggerSelector option for this.

  2. To properly check js expressions inside attributes we might need to have/build an AST that we can traverse. For this rule, when checking if an expression references a variable from script we need to traverse the element parents and check if v-for, slot-scope or something else defines variables already. We could for example use the AST from pug-parser and add some more data for expression contents.

Shinigami92 commented 2 years ago

I encountered a few issues which I'd like feedback on before I start to change code:

  1. This rule needs to be called on Programm instead of Programm:exit because it influences another existing rule. This is currently just hacked in here #158 (files) and obviously needs to be fixed properly. We could add an option to processRule. vue-eslint-parser has an templateBodyTriggerSelector option for this.
  2. To properly check js expressions inside attributes we might need to have/build an AST that we can traverse. For this rule, when checking if an expression references a variable from script we need to traverse the element parents and check if v-for, slot-scope or something else defines variables already. We could for example use the AST from pug-parser and add some more data for expression contents.
  1. For now it sounds okay for me if it works I didn't touched the code that much the last ~3 month, but remember that I implemented a caching system. If this is not affected and just work with a switch between Program and Program:exit, then go for it -> what ever it needs :slightly_smiling_face:

  2. I didn't try the ast version of pug yet at all, because I wanted to safe the processing time. Not sure if a ast is helpful anyways due do everything in pug can be handled iteratively.

Let me try to explain:

https://github.com/Shinigami92/eslint-plugin-vue-pug-sfc/blob/e6f6616581823a7a88c7db11115a9feff7964499/src/rules/valid-v-else-if.ts#L30-L32

Just one random rule, I took some variables and when I encounter it, I safe it. Then when I need it, I access it. But the whole processing is 99% from left to right, from top to bottom. And due to pug is based on indent-nesting, we also can take this into account and clean variables when the nesting reached out its level :raised_hands: First try to find a way to use this strategy and only start thinking about ast if we really really need it. But if so, I expect much slower processing and performance loose.

Shinigami92 commented 2 years ago

Oh and I head a look into my other open PRs if there is anything that is not directly related to a specific rule and can be used: found this: https://github.com/Shinigami92/eslint-plugin-vue-pug-sfc/pull/142/files#diff-bc03228a6e8ed14e4914c94dabefdcc8b119036acc8f6ada2f4d5fc59c751589 src/utils/js-reserved.ts

So if you need that, just copy it over :slightly_smiling_face:

rashfael commented 2 years ago

Just one random rule, I took some variables and when I encounter it, I safe it. Then when I need it, I access it. But the whole processing is 99% from left to right, from top to bottom. And due to pug is based on indent-nesting, we also can take this into account and clean variables when the nesting reached out its level raised_hands First try to find a way to use this strategy and only start thinking about ast if we really really need it. But if so, I expect much slower processing and performance loose.

Pug has quite a few feature like nested tags or tag interpolation which make tracking parents and their attributes a bit more difficult. I can see having a stack of parents with variables working, but getting all logic involved in parsing pug tokens right seems a lot more difficult than relying on a library from the pug team. Since you've built the pug plugin for prettier, perhaps you already have a solution in mind for handling stuff like nested tags and tag interpolation?

Shinigami92 commented 2 years ago

Since you've built the pug plugin for prettier, perhaps you already have a solution in mind for handling stuff like nested tags and tag interpolation?

The plugin does exactly what I described above :) writing the code from left to write from top to bottom I only use the lexer, not the ast for exactly the same reason: performance

I know it's a bit more difficult, but I think it's archivable.

[...] relying on a library from the pug team

I must say that I'm very disappointed right now from the "pug-team", because there is no active development involved around pug core at all :slightly_frowning_face: They look into the repo like only ever 4+ months or so and even then do not do anything... Just one example: https://github.com/pugjs/pug/issues/3347

So as an additional reason: the less dependencies relying on pug-ecosystem itself, the less audit issues can be involved in the future...

But still: I'm not saying I would deny anything, just want to give my opinions around that. If you really think it's worth the effort to use the pug-parser and the performance will not suffer from it, feel free to open a PR

rashfael commented 2 years ago

The plugin does exactly what I described above :) writing the code from left to write from top to bottom I only use the lexer, not the ast for exactly the same reason: performance

I know it's a bit more difficult, but I think it's archivable.

I believe you and I agree that we don't necessarily need an AST, I'm just asking if you already have code somewhere for tracking "open tags" or if I need to write that myself (including tracking : tokens and interpolation).

I must say that I'm very disappointed right now from the "pug-team", because there is no active development involved around pug core at all slightly_frowning_face They look into the repo like only ever 4+ months or so and even then do not do anything... Just one example: pugjs/pug#3347

Ah, that sucks, I didn't know that.

But still: I'm not saying I would deny anything, just want to give my opinions around that. If you really think it's worth the effort to use the pug-parser and the performance will not suffer from it, feel free to open a PR.

I'll try it with just the lexer first.

Shinigami92 commented 2 years ago

I believe you and I agree that we don't necessarily need an AST, I'm just asking if you already have code somewhere for tracking "open tags" or if I need to write that myself (including tracking : tokens and interpolation).

:+1: , but no I don't have such code

rashfael commented 2 years ago

I am reading through vue-eslint-parser to see what is needed to parse js expressions and that's a lot of things we would need to adapt/rebuild (and a lot of testing surface). I would not say it's impossible, but if we can avoid this, we probably save ourselves a lot of pain.

I started a PR at vue-eslint-parser to perhaps make custom template tokenizers configurable, so they don't need to add pug support directly inside vue-eslint-parser. We could then use that interface to add a pug tokenizer and get all that attribute and expression processing for "free".

rashfael commented 2 years ago

solved in #162