-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support for importing functions from tailwind plugins #5121
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
Conversation
@adhami3310 Do we need any tests? I have fixed the tailwind plugins installation too. |
CodSpeed Performance ReportMerging #5121 will not alter performanceComparing Summary
|
@adhami3310 It should work now. |
looks good! we should have a test for it and otherwise we can merge it in |
@adhami3310 I have added 2 test cases and rewrote |
why? i'm not sure i can get the advantage here |
@adhami3310 The test cases were failing because the function |
@adhami3310, why are integration tests failing? I have run it locally, it is running perfectly fine. Most of them are passed, except for the reflex web and Redis (I used to run the same pytest command in CONTRIBUTING.md), which I see here. I don't know what to do next. |
reflex web does something like {
plugins: ["@tailwindcss/typography", "tailwindcss-radix"],
darkMode: "class",
theme: {
extend: {
colors: {
gray: {
1: "var(--gray-1)",
2: "var(--gray-2)",
3: "var(--gray-3)",
4: "var(--gray-4)",
5: "var(--gray-5)",
6: "var(--gray-6)",
7: "var(--gray-7)",
8: "var(--gray-8)",
9: "var(--gray-9)",
10: "var(--gray-10)",
11: "var(--gray-11)",
12: "var(--gray-12)",
}
},
},
},
} |
@adhami3310 I have added a quickfix to jinja template. I think it should work (hopefully). If succeed we can merge it. I have written the jinja template, it works perfectly (when I used reflex-web's tailwind config) but remains generates an unformatted tailwind config in |
@adhami3310 I think issue is resolved now; pre-commit was the only failing check which I have fixed now. You can merge right in. |
reflex/utils/format.py
Outdated
if library_fullname.startswith("@") and "/" not in library_fullname: | ||
# Check for a second @ symbol | ||
second_at_index = library_fullname.find("@", 1) | ||
if second_at_index != -1: |
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.
it seems this code effectively does the same thing, why is it being changed?
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.
TLDR: The updated diff parses packages faster than previous one.
The updated implementation in the diff is faster and more robust because it adds various optimizations and fast paths to handle specific cases. Here's why:
-
Fast Path for URLs:
- The line
if library_fullname.startswith("https://"): return library_fullname
is executed early if the input is a URL, avoiding unnecessary processing.
- The line
-
Fast Path for Non-String Inputs:
- The updated version includes a check for non-string inputs using
not isinstance(library_fullname, str)
. If the input isn't a string, it quickly converts it to a string and returns it, saving unnecessary computations.
- The updated version includes a check for non-string inputs using
-
Fast Path for Packages Without '@':
- The line
if "@" not in library_fullname: return library_fullname
ensures that for package names without a version (e.g.,"foo"
), the function skips further processing and directly returns the input.
- The line
-
Special Case Handling:
- The new implementation handles edge cases like
@library@version
(no slash but starts with@
) and scoped packages (@scope/package@version
) more efficiently with targeted checks and substring operations.
- The new implementation handles edge cases like
-
Early Returns:
- The updated implementation introduces multiple early return paths, reducing the need for deeper logic or unnecessary processing.
In contrast, the old implementation processes all inputs uniformly, even if they don't contain a version specifier or don't require parsing. The new implementation is faster because it avoids unnecessary string operations and adds targeted checks for common scenarios.
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.
That's not how Python works. A singular operation is more likely to be faster than a multitude of operations that do the same thing.
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.
That's not how Python works. A singular operation is more likely to be faster than a multitude of operations that do the same thing.
Yup. I was thinking the same thing while replying. I felt strange when Claude 3.7 Sonnet (Github Copilot) replied with this, when I cross checked.
@adhami3310 I went back to original with dict support. |
@adhami3310 The checks are failing because of webpack errors. |
|
thanks! Are you going to ship before tailwind v4 or along with tailwind v4? |
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features:
🧩 Example Python Data Input
Here’s how you’d pass the data for plugins like
heroui()
andthemeVariants(...)
:🧠 Why This Is Better
require()
orconst { fn } = require('...')
imports.