-
-
Notifications
You must be signed in to change notification settings - Fork 503
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(js_semantic): support jsxFactory
and jsxFragmentFactory
#3761
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #3761 will not alter performanceComparing Summary
|
|
||
impl Default for JsxFactory { | ||
fn default() -> Self { | ||
Self("React".to_string()) |
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.
JsxFactory
only stores its namespace. We eagerly parse the expression to support configuration validation.
Using React
instead of React.createElement
or React.Fragment
to make it more performant.
} | ||
} | ||
|
||
fn parse_jsx_factory(value: &str) -> Option<JsxFactory> { |
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 know if this is the right place to put, thinking of this is a quite general utility.
@@ -189,8 +192,16 @@ impl UnresolvedReference { | |||
&self.data.binding_node_by_start[&reference.range.start()] | |||
} | |||
|
|||
pub fn tree(&self) -> AnyJsIdentifierUsage { |
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 will be considered as a breaking change as the binding does not guarantee it to be a JsIdentifier
anymore. It could be a JSX element or a JSX fragment. Also biome_js_semantic
releases the crate.
I'm also thinking of adding a test case for configuration. It will be great if someone can tell me the best place to put it. |
@ematipico Hi! This PR is ready for review. But it contains some breaking changes which seems hard to avoid. Would you please help me see if there's a better choice to do that? ❤️ |
Hi @h-a-n-a, If I understand correctly, you create a binding between the imported I understand what you are trying to achieve here, but I have mixed feelings because it is not really a binding: usually a binding binds a declaration to its usages. Here the usage will be generated by a compiler ; it is not present yet. We could perhaps create a dedicated event for this use case. Let me think about it and come back later this week when I have a clearer view. |
I had the same feeling too when I was implementing this feature. I had to do all kinds of breaking changes to make this work even like casting the Thank you for your reply and I'd be happy to fix this in the future. |
Summary
closes #3682
Supported option
javascript.jsxFactory
andjavascript.jsxFragmentFactory
.For
javascript.jsxFactory
, it controls the JSX factory name and forjavascript.jsxFragmentFactory
, it controls the JSX fragment factory name. Both of them work as long asjavascript.jsxRuntime
is set toReactClassic
.JSX usages are now scope-aware. It can track in-scope usages.
Changes to rule
noUnusedImport
For input,
with option:
Previously:
Both
h
andFragment
are marked as unused.Currently:
Both
h
andFragment
are marked as used.Changes to rule
noUndeclaredVariables
For input,
with option:
Previously:
Linter will not complain. As, by default, for
"reactClassic"
, we want to make it compatible with both modern and legacy compilers.Currently:
Linter reads
jsxFactory
andjsxFragmentFactory
. It will complain bothh
andFragment
are not imported. IfjsxFactory
andjsxFragmentFactory
are resolved to namespaceReact
, linter will not complain for compatibility.Checklist
jsx_factory
andjsx_fragment_factory
support forSemanticEventExtractor
(scope-aware)jsx_factory
andjsx_fragment_factory
BREAKING CHANGES
UnresolvedReference::tree
is removed as the binding does not guarantee it to be a JsIdentifier anymore. It could be a JSX element or a JSX fragment.SemanticEventExtractor::enter
added a context for passing downjsxFactory
andjsxFragmentFactory
. It's a little bit weird to add. I would be grateful if you could help me find a better solution.Test Plan
Tests are added.