fix: Fix withUt in monorepos #987
Conversation
🦋 Changeset detectedLatest commit: 9e5d416 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
.changeset/shiny-singers-live.md (1)
1-5: Changeset looks good, with a minor suggestion for improvement.The changeset file correctly documents the patch update for the "uploadthing" package, which aligns with the PR objectives. The use of a patch version is appropriate for this bug fix.
Consider expanding the change description slightly to provide more context. For example:
-Fixed withUt to work in monorepos +Fixed withUt to work in monorepos by adjusting module resolution processThis addition gives users a bit more insight into the nature of the fix without going into unnecessary technical details.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .changeset/shiny-singers-live.md (1 hunks)
- packages/uploadthing/src/tw.ts (1 hunks)
Additional comments not posted (1)
packages/uploadthing/src/tw.ts (1)
13-16: Approve changes with suggestions for improvementThe modification to
require.resolvealigns well with the PR objectives, addressing the issue withwithUtin monorepos by incorporating the current working directory in the module resolution process. This change should improve the reliability of module resolution in various monorepo setups.Here are some suggestions to further improve the code:
Add a comment explaining the reasoning behind this change, e.g.:
// Include process.cwd() to support module resolution in monoreposConsider potential edge cases:
- What happens if
process.cwd()is already included inmodule.paths?- Are there any performance implications of adding an extra path to resolve?
It would be beneficial to verify this change in different monorepo setups to ensure it works as expected across various configurations.
To verify the impact and effectiveness of this change, you can run the following script:
This script will help identify potential areas of impact and verify that the change is comprehensive across the project.
| "./src/**/*.{js,jsx,ts,tsx}", | ||
| "./node_modules/@uploadthing/solid/dist/**/*.js", // Remove this once #975 is resolved | ||
| ], | ||
| content: ["./src/**/*.{js,jsx,ts,tsx}"], |
There was a problem hiding this comment.
💡 Codebase verification
Potential Issue with Removing @uploadthing/solid Imports
The removal of the @uploadthing/solid entry from the content array may lead to missing styles or functionalities in components that rely on this package. Given that several files, including examples and documentation, still import @uploadthing/solid, it's crucial to ensure that this change does not adversely affect the project's styling or functionality.
🔗 Analysis chain
Simplified content array: Potential concerns and clarifications needed
The simplification of the content array to focus only on the project's source files is a good step towards improving the build process in a monorepo context. This change aligns with the PR objective of fixing withUt in monorepos.
However, I have a few points to address:
-
Can you confirm that removing the
@uploadthing/solidentry from the content array won't cause any missing styles or functionality? It's important to ensure that all necessary styles are still being included. -
The PR objectives mention modifying the
require.resolvebehavior, but this change doesn't directly address that. Could you clarify how this configuration change relates to therequire.resolvemodification mentioned in the PR description?
To ensure no critical styles are missing, let's check for any @uploadthing/solid imports in the project:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for @uploadthing/solid imports in the project
# Expected result: No imports found, confirming the removal is safe
rg --type js --type ts '@uploadthing/solid'
Length of output: 372
| // to be some local workspace issue, making pnpm fail to resolve `@uploadthing/react` | ||
| "./node_modules/@uploadthing/react/dist/**/*.js", | ||
| ], | ||
| content: ["./src/**/*.{js,mjs,jsx,ts,tsx,mdx}"], |
There was a problem hiding this comment.
💡 Codebase verification
Tailwind classes with ut- prefix are still present in the codebase.
The presence of ut- prefixed Tailwind classes suggests that classes from @uploadthing/react might still be in use. Please review these instances to ensure they align with the intended changes and do not reintroduce the issues the PR aims to resolve.
🔗 Analysis chain
LGTM! Simplified content array may resolve monorepo issues.
The simplification of the content array to only include files from the ./src directory is a good practice and aligns with the PR objectives to fix issues with withUt in monorepos. Removing the commented-out node_modules path should help resolve the workspace resolution problems mentioned in the AI summary.
To ensure this change doesn't negatively impact the functionality of withUt, please run the following verification script:
If the script shows that withUt is still being imported and used, and no Tailwind classes from @uploadthing/react are being directly used in the project, then this change should be safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that withUt is still being properly imported and used after the content array change.
# Test: Check if withUt is still imported and used in the project
rg --type typescript 'import.*withUt' ./src
rg --type typescript 'withUt\(' ./src
# Test: Verify that no Tailwind classes from @uploadthing/react are being used in the project
rg --type typescript --type javascript --type jsx --type tsx 'className="[^"]*ut-[^"]*"' ./src
Length of output: 287
Script:
#!/bin/bash
# Description: Verify that withUt is still being properly imported and used after the content array change.
# Test: Check if withUt is still imported in the project
rg --files -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -g "*.mjs" -g "*.cjs" | xargs rg 'import\s+.*withUt'
# Test: Check if withUt is being used in the project
rg --files -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -g "*.mjs" -g "*.cjs" | xargs rg 'withUt\('
# Test: Verify that no Tailwind classes from @uploadthing/react are being used in the project
rg --files -g "*.ts" -g "*.tsx" -g "*.js" -g "*.jsx" -g "*.mjs" -g "*.cjs" | xargs rg 'className="[^"]*ut-[^"]*"'
Length of output: 2944
| const resolved = require.resolve(`@uploadthing/${pkg}`); | ||
| return resolved.split(sep).slice(0, -1).join(sep) + `${sep}**`; | ||
| const resolved = require.resolve(`@uploadthing/${pkg}`, { | ||
| paths: [...module.paths, process.cwd()], |
There was a problem hiding this comment.
Thank you for this, I probably would have spent quite some time to think of trying this xd
2 participants
Second attempt at fixing #975. I believe this should work. Tested in solidstart in monorepo, and outside of monorepo.
I hope this does what I think it does, but the idea is to add current working directory via
process.cwd()to pathsrequire.resolvelooks for.Summary by CodeRabbit
withUtfeature, improving its functionality in monorepo environments.uploadthingfunctionality, focusing on compatibility within monorepos.