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

fix(lint): linting package with .css file should raise no errors #26695

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cli/graph_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,10 @@ impl ModuleGraphCreator {

let mut roots = Vec::new();
for package_config in package_configs {
eprintln!(
"resolve export {:?}",
package_config.config_file.resolve_export_value_urls()?
);
roots.extend(package_config.config_file.resolve_export_value_urls()?);
}
let mut graph = self
Expand All @@ -276,7 +280,9 @@ impl ModuleGraphCreator {
loader: None,
})
.await?;
self.graph_valid(&graph)?;
let r = self.graph_valid(&graph);
Copy link
Member Author

Choose a reason for hiding this comment

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

This call should filter out "unsupported media type" errors when graph is used for linting

eprintln!("result {:#?}", r);
r?;
if self.options.type_check_mode().is_true()
&& !graph_has_external_remote(&graph)
{
Expand Down
6 changes: 5 additions & 1 deletion cli/tools/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ impl WorkspaceLinter {
deno_lint_config: lint_config,
}));

eprintln!("paths {:#?}", paths);
let mut futures = Vec::with_capacity(2);
if linter.has_package_rules() {
// Package rules are only in effect if user didn't explicitly specify paths on the command line
if paths.is_empty() && linter.has_package_rules() {
Comment on lines +296 to +299
Copy link
Member Author

Choose a reason for hiding this comment

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

This bit seems broken - whether we invoke deno lint or deno lint main.ts in a directory that has deno.json, the paths contains main.ts. We were unconditionally running the "package rules" lints as long as the deno.json contains "publish config". Changing this conditional effectively breaks package lints. I think this whole bit should be rewritten to account for the fact that we're running lint for a whole dir or a certain file.

if self.workspace_module_graph.is_none() {
let module_graph_creator = self.module_graph_creator.clone();
let packages = self.workspace_dir.jsr_packages_for_publish();
Expand Down Expand Up @@ -323,9 +325,11 @@ impl WorkspaceLinter {
.collect::<HashSet<_>>();
futures.push(
async move {
eprintln!("workspace module graph");
let graph = workspace_module_graph_future
.await
.map_err(|err| anyhow!("{:#}", err))?;
eprintln!("workspace module graph2");
let export_urls =
publish_config.config_file.resolve_export_value_urls()?;
if !export_urls.iter().any(|url| path_urls.contains(url)) {
Expand Down
9 changes: 6 additions & 3 deletions cli/tools/lint/rules/no_slow_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,16 @@ pub fn collect_no_slow_type_diagnostics(
graph: &ModuleGraph,
package_export_urls: &[ModuleSpecifier],
) -> Vec<FastCheckDiagnostic> {
let mut js_exports = package_export_urls
let mut js_exports: Vec<_> = package_export_urls
.iter()
.filter_map(|url| graph.get(url).and_then(|m| m.js()));
.filter_map(|url| graph.get(url).and_then(|m| m.js()))
.collect();

eprintln!("js exports {:#?}", js_exports);
// fast check puts the same diagnostics in each entrypoint for the
// package (since it's all or nothing), so we only need to check
// the first one JS entrypoint
let Some(module) = js_exports.next() else {
let Some(module) = js_exports.into_iter().next() else {
// could happen if all the exports are JSON
return vec![];
};
Expand Down
12 changes: 12 additions & 0 deletions tests/specs/lint/package_with_css_export/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"tests": {
"run_dir": {
"args": "lint",
"output": "output.out"
},
"run_file": {
"args": "lint main.ts",
"output": "Checked 1 file\n"
}
}
}
7 changes: 7 additions & 0 deletions tests/specs/lint/package_with_css_export/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@project/ui",
"exports": {
"./main.ts": "./main.ts",
"./globals.css": "./globals.css"
}
}
3 changes: 3 additions & 0 deletions tests/specs/lint/package_with_css_export/globals.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
body {
color: "red";
}
Empty file.
Empty file.
Loading