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(linter): support lint vue files #1706

Closed
wants to merge 1 commit into from

Conversation

mysteryven
Copy link
Contributor

closes: #1238

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@github-actions github-actions bot added the A-linter Area - Linter label Dec 17, 2023
@mysteryven mysteryven force-pushed the 12-17-feat_linter_support_lint_vue_files branch from 66ddc97 to 2322730 Compare December 17, 2023 08:25
}
fn may_parser_special_extension<P: AsRef<Path>>(path: P) -> (Option<bool>, Option<Span>) {
let default_ret = (None, None);
let Ok(content) = std::fs::read_to_string(path.as_ref()) else { return default_ret };
Copy link
Contributor Author

@mysteryven mysteryven Dec 17, 2023

Choose a reason for hiding this comment

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

I don't really like here, it's means we need to read the content of Vue file twice. One here, another is in process_path

let mut source_text = match fs::read_to_string(path) {
Ok(source_text) => source_text,
Err(e) => {
tx_error
.send(Some((
path.to_path_buf(),
vec![Error::new(FailedToOpenFileError(path.to_path_buf(), e))],
)))
.unwrap();
return;
}
};

How should I improve it? Where should I put SpecialLanguageVariantParser in?😥

@mysteryven mysteryven marked this pull request as draft December 17, 2023 09:37
@mysteryven mysteryven force-pushed the 12-17-feat_linter_support_lint_vue_files branch from 2322730 to 0aa3c2c Compare December 17, 2023 13:38
@Boshen
Copy link
Member

Boshen commented Dec 17, 2023

The architecture around this is a bit more involved I think. We can support .astro if this is done right.

Let's iterate on this instead of having a huge PR.

@mysteryven mysteryven force-pushed the 12-17-feat_linter_support_lint_vue_files branch from 0aa3c2c to ca824dc Compare December 18, 2023 04:10
Copy link

codspeed-hq bot commented Dec 18, 2023

CodSpeed Performance Report

Merging #1706 will improve performances by 5.9%

Comparing 12-17-feat_linter_support_lint_vue_files (ca824dc) with main (edc6fa4)

Summary

⚡ 1 improvements
✅ 19 untouched benchmarks

Benchmarks breakdown

Benchmark main 12-17-feat_linter_support_lint_vue_files Change
semantic[vue.js] 80.9 ms 76.4 ms +5.9%

@aparajita
Copy link

Eagerly awaiting this! 🙏

let attributes_text =
Span::new(open_tag_start, open_tag_end).source_text(self.source_text);
is_ts = attributes_text.contains(r#"lang="ts""#)
|| attributes_text.contains("lang='ts'");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with Vue, but I believe there might be another case here:

<script lang=ts>

let is_definition_file = file_name.ends_with(".d.ts")
|| file_name.ends_with(".d.mts")
|| file_name.ends_with(".d.cts");

let language = match extension {
"js" | "mjs" | "cjs" | "jsx" => Language::JavaScript,
"ts" | "mts" | "cts" | "tsx" => Language::TypeScript { is_definition_file },
"vue" => {

Choose a reason for hiding this comment

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

I'm prretty sure the same code will solve the case for svelte, as requested in #1770

  • Svelte should use the same <script lang="ts"> style
Suggested change
"vue" => {
"vue" | "svelte" => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I am planing to support svelte in another way.

@mysteryven
Copy link
Contributor Author

Will continue work on #1814

@Boshen
Copy link
Member

Boshen commented Dec 31, 2023

vue support has been merged as a stack, see #1863 (comment)

Feel free to join, test and improve it together.

@Boshen Boshen closed this Dec 31, 2023
@Boshen Boshen deleted the 12-17-feat_linter_support_lint_vue_files branch December 31, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(linter): Support lint the <script> part of Vue file
5 participants