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

Custom template tokenizers #148

Merged
merged 18 commits into from Apr 14, 2022
Merged

Conversation

rashfael
Copy link
Contributor

@rashfael rashfael commented Mar 14, 2022

This PR adds support for custom template tokenizers and can be used to add pug support externally.

Custom template tokenizers can be configured in parserOptions:

parserOptions: {
  templateTokenizer: {
    pug: "vue-eslint-parser-template-tokenizer-pug"
  }
}

The custom tokenizer generates IntermediateTokens like the ones generated by the IntermediateTokenizer, which then get consumed by the html parser. I think this is the easiest way to not have the custom tokenizer worry about attribute/directive parsing.

There is still some functionality missing but what do you think about adding pug support in this way?

TODOs:

  • replace default tokenizer tokens errors comments with ones from the custom tokenizer.
  • Add tests
  • Investigate tokenizer.expressionEnabled

Refs #29

I started an implementation for a pug template tokenizer here: https://github.com/rashfael/vue-eslint-parser-template-tokenizer-pug

@ota-meshi
Copy link
Member

Thank you for this PR.
I think the idea of allowing the template parser to be extended is good.
But I still don't know how the options you added will work. Can you explain what kind of input produces what kind of AST? Or add test cases.

@ota-meshi
Copy link
Member

Also, can you tell me why it's better than eslint-plugin-vue-pug-sfc when it works with Pug?

@ota-meshi
Copy link
Member

By the way, I don't use Pug, so I'm not familiar with Pug.

@rashfael
Copy link
Contributor Author

Thank you for this PR. I think the idea of allowing the template parser to be extended is good.

That's great to hear!

But I still don't know how the options you added will work. Can you explain what kind of input produces what kind of AST? Or add test cases.

I'll add test cases for sure.

The templateTokenizer option behaves similar to the parser option that already exists, where you can specify an object with keys for each lang in <script> tags. The value from each key is a package name which will be required to load the tokenizer, just like in the existing parser option.

I'll document how the interface will look, it's something like this 9720b51#diff-3bb9c55886cd92e12d33790e215997dbd9a7e91b339459b52726f51ce9b0a017R6 .

The custom template tokenizer receives the raw text string from inside the <template> tag (when the lang attribute matches). The html.Parser then generates tokens from the custom tokenizer by calling nextToken, which returns a IntermediateToken. The custom tokenizer does not return a full AST but instead has the same API the internal html tokenizer has. By doing it this way, the custom tokenizer can leave more complex parsing to the HTML parser, like processing attributes into directives and parsing inline js expressions.

Also, can you tell me why it's better than eslint-plugin-vue-pug-sfc when it works with Pug?

Mainly for the same reason I opted for emitting tokens instead of a full AST in this PR. Implementing all the functionality vue-eslint-parser offers would mean reimplementing a lot of code in eslint-plugin-vue-pug-sfc, especially everything regarding parsing js expressions in the template. eslint-plugin-vue-pug-sfc could actually use this custom tokenizer to implement rules specific to pug without the need to rebuild every rule from eslint-plugin-vue.

I only tested the attribute-hyphenation rule from eslint-plugin-vue, but that rule works transparently without any modification for pug templates. I'll test other rules once the customer tokenizer creates all needed tokens.

I will continue working on this PR and adding tests.

@ota-meshi
Copy link
Member

I plan to make breaking changes to the <script setup>. #144
Should I merge this first to prevent conflicts?

@rashfael
Copy link
Contributor Author

Should I merge this first to prevent conflicts?

Do you mean merging #144 first or #148 ?

Merging #144 first works for me, I can then resolve conflicts in this PR.

@ota-meshi
Copy link
Member

I will merge #144 😅

@Shinigami92
Copy link

@rashfael @ota-meshi ping me if you need a review or maybe answers for some questions if there are some.

@rashfael
Copy link
Contributor Author

I have documented the interface and wrote a test with a mock tokenizer. which just outputs a hardcoded set of tokens. The mock tokenizer gets some locations wrong, but I think for testing purposes this does not matter much. I will still try to fix the wrong locations.

I think I have accounted for all possible features. I will continue writing the actual pug tokenizer and testing eslint-plugin-vue rules which may reveal something I missed.

@Shinigami92, a review of my changes and feedback would be much appreciated.

@Shinigami92
Copy link

I had a look over code and docs, don't fully understand yet everything due to I didn't worked with it in action, but I also currently do not have the time to go so much in depth, so I trust your code.
Also if there is something we need to change, this is maybe just the starting point 🙂
So let's run the test, ship it and try it out 🙂

@rashfael rashfael marked this pull request as ready for review March 22, 2022 19:42
@rashfael
Copy link
Contributor Author

Just a quick status update:
I have autogenerated pug-equivalent tests for existing rules from eslint-plugin-vue and it's looking promising so far. I just have to do a lot of manual work updating the expected error lines and columns, since these obviously differ from the ones for plain html.

I find it unlikely that I will find any problem with the API implemented in this PR.
All issues I found so far are bugs in my tokenizer.
So, I would call this PR ready and would like your feedback @ota-meshi . Is there something missing for this PR to be merged?

@Shinigami92 do you want to discuss how we want to proceed with pug linting in general?

@Shinigami92
Copy link

@Shinigami92 do you want to discuss how we want to proceed with pug linting in general?

Uhm yeah, we could do this in a GitHub discussion somewhere 🤔
I'm not up-to-date due to hardly working on @faker-js/faker
Maybe I could send you somehow privately my Discord name? Or you can try find me in some popular Discord servers and add me from there. (Vue, Vite, faker, cypress, quasar, ...)

@rashfael
Copy link
Contributor Author

I have continued my work at eslint-plugin-vue-pug and all base and essential rules are running out of the box, only one rule needed a replacement. I'll continue with testing all existing vue/* rules.

I also tested the whole setup on a large real world project of mine and linting is working well.

@ota-meshi are you happy with this PR?

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

I don't have time to look at this PR right now. I found one, so I will comment.

@ota-meshi
Copy link
Member

Could you change the defineTemplateBodyVisitor and defineDocumentVisitor to not validate templates other than lang=html or no lang by default? Also, could you add options so that we can specify the languages to visit?
I think the option can prevent the rule from unexpected behavior in untested languages (e.g. pug).

@rashfael
Copy link
Contributor Author

rashfael commented Apr 5, 2022

Having the default behaviour ignore custom languages like pug kinda defeats the purpose of being able to just re-use existing rules. As it is right now, over 90% of the rules from eslint-plugin-vue just work without any changes. If we would introduce additional parameters to exclude/include langs, I would need to fork all rules.

@rashfael
Copy link
Contributor Author

rashfael commented Apr 5, 2022

Additionally, this PR won't change any behaviour as long as you don't configure a templateTokenizer in the parserOptions for the language you want different behaviour for, so a vanilla eslint-plugin-vue will be unaffected.

@ota-meshi
Copy link
Member

ota-meshi commented Apr 6, 2022

Do you say that eslint-plugin-vue will remain officially unsupported for Pug?
If so, I think we need to add more detail to the documentation about the risks of using the templateTokenizer option.
(It may also be better to state that we do not guarantee anything other than vue-eslint-parser-template-tokenizer-pug.)

the purpose of being able to just re-use existing rules

I thought that only the eslint-plugin-vue rule that added the Pug test (and use visit option of defineTemplateBodyVisitor) should work with Pug. If you don't need to officially support Pug, I think the method as you said is ok.

@rashfael
Copy link
Contributor Author

rashfael commented Apr 6, 2022

Do you say that eslint-plugin-vue will remain officially unsupported for Pug?

Yes, I was thinking eslint-plugin-vue and vue-eslint-parser do not need to have any specific pug code and don't need to support it.
eslint-plugin-vue-pug will handle pug compatibility and uses vue-eslint-parser-template-tokenizer-pug to make most of the eslint-plugin-vue rules work out of the box. eslint-plugin-vue-pug has tests for all eslint-plugin-vue rules to check if they work with pug here and has some replacements if there are problems.

If so, I think we need to add more detail to the documentation about the risks of using the templateTokenizer option. (It may also be better to state that we do not guarantee anything other than vue-eslint-parser-template-tokenizer-pug.)

Sounds good, I'll add a disclaimer that this setting is intended for plugin developers and if you are looking for pug support, you should use eslint-plugin-vue-pug.

I thought that only the eslint-plugin-vue rule that added the Pug test (and use visit option of defineTemplateBodyVisitor) should work with Pug. If you don't need to officially support Pug, I think the method as you said is ok.

In vue-eslint-parser I only added a test to check if the custom tokenizer abstraction is working, in eslint-plugin-vue-pug I am testing and tracking every eslint-plugin-vue rule if they work with concrete pug templates.

@Shinigami92
Copy link

Shinigami92 commented Apr 6, 2022

eslint-plugin-vue-pug

I also communicated with @rashfael, and when eslint-plugin-vue-pug works, I will archive my eslint-plugin-vue-pug-sfc repo and redirect to the "new" home 🙂

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

I made only one comment on the source code.

I think we should add more notes to the document. I think that the specification of the templateTokenizer option will change even in the minor version, so I think it is good to mark it as an experimental feature.

src/html/parser.ts Outdated Show resolved Hide resolved
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM

@ota-meshi ota-meshi merged commit 9448a78 into vuejs:master Apr 14, 2022
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.

None yet

3 participants