-
Notifications
You must be signed in to change notification settings - Fork 377
Extract shared utilities into workspace package #5843
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
🎭 Playwright Test Results⏰ Completed at: 09/29/2025, 12:05:21 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
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.
Utils is a little generic, yeah.
It feels like it would be hard to define a bright line inclusion rule for it.
Is there a way to hint to git that the files are a move/copy?
What's up with createAnnotatedPath?
Yeah super not happy with it. Thought of a bunch of really bad alternatives. Didn't want to prefix with
So it displays on the "Files changed" tab (without viewing in ranges)? Not without removing the shims / adding them in a follow-up PR. I explicitly |
It uses the following dependency chain: ResultItem -> split schemas out to package -> i18n -> nope. Path of least resistance was to leave it behind. |
Test failure: confirmed unrelated. |
tsconfig.json
Outdated
"@/*": ["src/*"], | ||
"@/utils/envUtil": ["src/utils/envUtil.shim.ts"], | ||
"@/utils/formatUtil": ["src/utils/formatUtil.shim.ts"], | ||
"@/utils/networkUtil": ["src/utils/networkUtil.shim.ts"] |
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.
Does this work?
"@/*": ["src/*"], | |
"@/utils/envUtil": ["src/utils/envUtil.shim.ts"], | |
"@/utils/formatUtil": ["src/utils/formatUtil.shim.ts"], | |
"@/utils/networkUtil": ["src/utils/networkUtil.shim.ts"] | |
"@/*": ["src/*"], | |
"@/utils/envUtil": ["packages/shared-frontend-utils/src/envUtil.ts"], | |
"@/utils/formatUtil": ["packages/shared-frontend-utils/src/formatUtil.ts"], | |
"@/utils/networkUtil": ["packages/shared-frontend-utils/src/networkUtil.ts"] |
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.
Yeah, I refactored down to this after the last commit. But the build failure is unrelated, and more painful.
I need to reduce the scope further; will either close PR or force push something else later.
fd044d2
to
a81ace8
Compare
## Summary Extracts shared formatting and network utilities into dedicated workspace package. ## Changes - **What**: Created `@comfyorg/shared-frontend-utils` package containing formatUtil and networkUtil - **Breaking**: None - utilities remain accessible via path aliases in `tsconfig` Split `createAnnotatedPath` and `electronMirrorCheck` out and left in frontend, due to their tightly-coupled nature. See [discussion on this PR](#5843 (comment)).
Summary
Extracts shared formatting and network utilities into dedicated workspace package.
Changes
@comfyorg/shared-frontend-utils
package containing formatUtil and networkUtiltsconfig
Split
createAnnotatedPath
andelectronMirrorCheck
out and left in frontend, due to their tightly-coupled nature. See discussion on this PR.Review Focus
Package naming - is
@comfyorg/shared-frontend-utils
appropriate for these utilities? Please think of something better, then tell me about it.(Edit: No more two-link views required - "discovered" a neat way to squash merge by using different file names and tsconfig aliases.. then just aliasing to the package files direct - just requires a root level tsconfig. Apparently, this is in the Nx docs. Spoilsports.)