fix: remove node env from build pipeline #1105
Conversation
🦋 Changeset detectedLatest commit: 7780e9e 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
|
|
Warning Rate limit exceeded@juliusmarminge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a patch for the "uploadthing" functionality, specifically addressing issues with automatic development mode detection based on the Changes
Possibly related PRs
Suggested reviewers
🪧 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 (
|
More templates
commit: |
📦 Bundle size comparison
|
![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
🧹 Nitpick comments (1)
.changeset/spotty-windows-fold.md (1)
1-5: Clarify the patch rationale for future reference.Including a short explanation of why removing
NODE_ENVfrom the pipeline resolves the dev mode inference issue can help others quickly understand the purpose of this patch in the future. Consider elaborating more in the changeset body.--- "uploadthing": patch --- + # Reason: + Removing `NODE_ENV` from the build pipeline ensures that the dev mode detection + code can automatically infer the environment without forced static replacements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/spotty-windows-fold.md(1 hunks)packages/uploadthing/src/internal/config.test.ts(1 hunks)packages/uploadthing/src/internal/config.ts(1 hunks)turbo.json(1 hunks)
🔇 Additional comments (3)
packages/uploadthing/src/internal/config.ts (1)
56-56: Ensure cross-environment compatibility.
By conditionally referencing process.env.NODE_ENV only when process is defined, you're making the code resilient to non-NodeJS runtimes. However, consider verifying that none of the bundling or hosting environments inadvertently remove or modify process references.
Run the following script to confirm there are no other unconditional references to process in the codebase:
✅ Verification successful
References to process are properly guarded in the codebase
After analyzing the codebase, I found that references to process are properly handled:
- In
packages/uploadthing/src/internal/config.ts, the code safely checks forprocessexistence:
typeof process !== "undefined" ? process.env.NODE_ENV : undefined- Other occurrences of
processare in:
- Test files (
*.test.ts) which run in Node.js environment - Configuration files (like
env.mjs,next.config.js) which are Node.js scripts - Example applications that use frameworks (Next.js, Expo) which handle
processreferences during build - Documentation files (
.mdx) showing example usage
The codebase demonstrates proper handling of environment variables across different runtime environments, with appropriate guards for non-Node.js environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find unconditional references to `process` across the codebase.
rg -A 3 "process\."
Length of output: 17739
packages/uploadthing/src/internal/config.test.ts (1)
280-280: Good coverage for dev mode detection using NODE_ENV.
The test ensures that development mode is accurately recognized when NODE_ENV is set. This addresses the scenario where NODE_ENV might not properly propagate in certain bundles. No issues detected.
turbo.json (1)
12-12: Confirm no breakage from removing NODE_ENV in the pipeline.
Removing NODE_ENV may impact scripts or build steps that previously relied on it. Verify that all tasks can still correctly detect dev vs. production mode if needed.
✅ Verification successful
Removing NODE_ENV from globalPassThroughEnv is safe
Based on the search results, NODE_ENV is primarily used in:
- Example apps' env.mjs files for environment validation
- Development scripts in backend-adapters examples
- UploadThing package for development mode detection
All these usages either:
- Access NODE_ENV directly from process.env
- Set NODE_ENV explicitly in npm scripts
- Have fallbacks when NODE_ENV is undefined
The removal from turbo.json's globalPassThroughEnv won't impact the codebase since:
- Build scripts explicitly set NODE_ENV when needed
- Runtime code properly accesses it through process.env
- Environment validation in examples uses process.env directly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check references to NODE_ENV in config and scripts to ensure they don't rely on it from the pipeline.
rg -A 5 "NODE_ENV"
Length of output: 7938
|
A new canary is available for testing. You can install this latest build in your project with: pnpm add @uploadthing/expo@7.1.5-canary.0063144
pnpm add @uploadthing/mime-types@0.3.4-canary.0063144
pnpm add @uploadthing/nuxt@7.1.5-canary.0063144
pnpm add @uploadthing/react@7.1.5-canary.0063144
pnpm add @uploadthing/shared@7.1.5-canary.0063144
pnpm add @uploadthing/solid@7.1.5-canary.0063144
pnpm add @uploadthing/svelte@7.1.5-canary.0063144
pnpm add uploadthing@7.4.3-canary.0063144
pnpm add @uploadthing/vue@7.1.5-canary.0063144 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/uploadthing/turbo.json (1)
6-6: LGTM! Consider documenting this change.The NODE_ENV exclusion is consistently implemented across all packages.
Consider adding a comment in the root turbo.json or README explaining why NODE_ENV is excluded from builds. This will help future maintainers understand the reasoning behind this configuration, especially regarding the development mode detection behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react/turbo.json(1 hunks)packages/shared/turbo.json(1 hunks)packages/uploadthing/turbo.json(1 hunks)turbo.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- turbo.json
🔇 Additional comments (2)
packages/shared/turbo.json (1)
6-6: LGTM! Verify schema validation.
The exclusion of NODE_ENV is correctly configured using the passThroughEnv property.
Let's verify the turbo.json schema to ensure the syntax is valid:
✅ Verification successful
Schema validation confirms correct syntax for passThroughEnv
Based on the schema definition, the passThroughEnv property in the task configuration is correctly defined. The schema shows that:
- It accepts an array of
EnvWildcarditems (string type) - Supports negation patterns (like "!NODE_ENV")
- Is an optional property with a default value of null
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Validate turbo.json schema
curl -s https://turbo.build/schema.json | jq -r '.properties.tasks.additionalProperties.properties.passThroughEnv'
Length of output: 119
Script:
#!/bin/bash
# Let's check the actual content of the schema to understand the correct property path
curl -s https://turbo.build/schema.json | jq '.'
Length of output: 14262
packages/react/turbo.json (1)
6-6: LGTM! Consistent with shared package configuration.
The NODE_ENV exclusion is implemented consistently across packages.
NODE_ENV being present during build causes static replacement and breaks automatic dev mode detection
Summary by CodeRabbit
NODE_ENVfrom being passed through during the build process across multiple packages.