-
-
Notifications
You must be signed in to change notification settings - Fork 485
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(graphql_analyze): useNamedOperation #4337
feat(graphql_analyze): useNamedOperation #4337
Conversation
aa33e72
to
ad310ac
Compare
CodSpeed Performance ReportMerging #4337 will not alter performanceComparing Summary
|
let mut mutation = ctx.root().begin(); | ||
let node = ctx.query().clone(); | ||
let new_name = make::graphql_name_binding(make::ident(&state.suggested_name)); | ||
let new_node = node.clone().detach().with_name(Some(new_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need detach
here?
markup! { | ||
"Anonymous GraphQL operations are forbidden. Make sure to name your " {state.operation_type}"!" | ||
}, | ||
) | ||
.note(markup! { | ||
"Rename this "{state.operation_type}" to "{state.suggested_name}"." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow our rule pillars https://biomejs.dev/linter/#rule-pillars
This means that the .note must become a message that explains the error. The core action is the solution for fixing the error.
} else { | ||
Some(NoAnonymousOperationsState { | ||
operation_type: operation_type.clone(), | ||
suggested_name: get_suggested_name(node, operation_type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should allocate strings only when needed. Here, we should do it in the action
function
|
||
fn run(ctx: &RuleContext<Self>) -> Option<Self::State> { | ||
let node = ctx.query(); | ||
let operation_type = node.ty().ok()?.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use text
because it allocates a string, which isn't needed. Instead, use syntax().text_trimmed()
} | ||
|
||
fn get_suggested_name(operation: &GraphqlOperationDefinition, operation_type: String) -> String { | ||
let suggested_name = get_suggested_name_base_on_content(operation).unwrap_or(operation_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the need for two separate functions, because they are used once. Let's put everything in one single function
use crate::GraphqlRuleAction; | ||
|
||
declare_lint_rule! { | ||
/// Require specifying name for GraphQL operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Require specifying name for GraphQL operations. | |
/// Enforce specifying the name of GraphQL operations. |
declare_lint_rule! { | ||
/// Require specifying name for GraphQL operations. | ||
/// | ||
/// This is useful since most GraphQL client libraries are using the operation name for caching purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// This is useful since most GraphQL client libraries are using the operation name for caching purposes. | |
/// This is useful because most GraphQL client libraries use the operation name for caching purposes. |
/// | ||
pub NoAnonymousOperations { | ||
version: "next", | ||
name: "noAnonymousOperations", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the documentation, and it seems the rule is more focused on the name of the operations. It doesn't say anything about operations that don't have a name.
Wouldn't it make more sense to call the rule useNamedOperation
? Plus, let's use the singular
rule_category!(), | ||
node.range(), | ||
markup! { | ||
"Anonymous GraphQL operations are forbidden. Make sure to name your " {operation_type.text()}"!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Anonymous GraphQL operations are forbidden. Make sure to name your " {operation_type.text()}"!" | |
"Anonymous GraphQL operations are forbidden. Make sure to name your " {operation_type.text()}"." |
}, | ||
) | ||
.note(markup! { | ||
"Most GraphQL client libraries use operation name for caching purposes." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Most GraphQL client libraries use operation name for caching purposes." | |
"Most GraphQL client libraries use the operation name for caching purposes." |
Some( | ||
RuleDiagnostic::new( | ||
rule_category!(), | ||
node.range(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think highlighting the whole query can be too much. You used very small examples, but if you enable the rule in a big project, the whole thing is going to be red, which isn't ideal for the users. What do you think if we highlight the operation instead?
b33f6a3
to
0769cd2
Compare
0769cd2
to
f3d0741
Compare
Disallow operations without name, since GraphQL clients usually use operations' name to cache.
f3d0741
to
535aa9d
Compare
Oops, my bad, didn't mean to re-request a review from you there 😅 |
Summary
Disallow operations without name, since GraphQL clients usually use operations' name to cache.
Test Plan
Added new tests to verify this rule's behavior