Skip to content
This repository was archived by the owner on Apr 19, 2022. It is now read-only.

Conversation

rashfael
Copy link
Collaborator

Reference #8

Continuing from #155

Mark as used:

  • Components
  • custom directives
  • ref values
  • dynamic expressions in directive values

@Shinigami92 Shinigami92 linked an issue Mar 13, 2022 that may be closed by this pull request
@rashfael
Copy link
Collaborator Author

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
Copy link
Owner

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 🙂

  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:

let hasVIf: boolean = false;
let vElseIfToken: AttributeToken | undefined;
let hasVElse: boolean = false;

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 🙌
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
Copy link
Owner

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 🙂

@rashfael
Copy link
Collaborator Author

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
Copy link
Owner

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 🙁
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

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
Copy link
Collaborator Author

rashfael commented Mar 13, 2022

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
Copy link
Owner

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).

👍 , but no I don't have such code

@Shinigami92 Shinigami92 changed the base branch from main to rule/script-setup-uses-vars March 13, 2022 17:22
@rashfael
Copy link
Collaborator Author

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
Copy link
Collaborator Author

solved in #162

@rashfael rashfael closed this Mar 23, 2022
@rashfael rashfael deleted the rule/script-setup-uses-vars branch March 23, 2022 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate script-setup-uses-vars
2 participants