Skip to content

Conversation

@christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Aug 27, 2025

Summary

Adds ADR-0003 documenting the architectural decision to fork PrimeVue as a monorepo workspace package. This ADR provides technical justification for why PrimeVue needs to be forked rather than used as an external dependency. Following from #5199.

Key Technical Issues Documented

  • Transform coordinate conflicts: PrimeVue components use getBoundingClientRect() + pageX/pageY calculations that break in CSS transform coordinate spaces
  • Virtual canvas scroll interference: PrimeVue's automatic scrollIntoView behavior disrupts LiteGraph's semantic scroll coordinate system
  • Historical overlay positioning issues: Previous z-index conflicts required manual workarounds

Review Focus

  • Technical accuracy of the documented issues
  • Alignment with existing ADR format and conventions
  • Completeness of the rationale for forking vs. alternatives

The ADR follows the established format from previous ADRs and includes specific technical details with file references and line numbers for verification.

┆Issue is synchronized with this Notion page by Unito

@christian-byrne christian-byrne requested a review from a team as a code owner August 27, 2025 16:38
Adds ADR-0003 documenting the decision to fork PrimeVue as a monorepo workspace package. Key rationale includes transform coordinate system conflicts and virtual canvas scroll interference that require component-level modifications.
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Aug 27, 2025
@github-actions
Copy link

github-actions bot commented Aug 27, 2025

🎭 Playwright Test Results

Tests completed successfully!

⏰ Completed at: 09/13/2025, 05:50:23 AM UTC

📊 Test Reports by Browser


🎉 Click on the links above to view detailed test results for each browser configuration.

@christian-byrne christian-byrne force-pushed the c_byrne/adr/primevue-fork branch from 1bfc976 to fa13516 Compare August 27, 2025 16:43
1. Add PrimeVue fork to `packages/@comfyui/primevue/` using git subtree
2. Use `--squash` flag to avoid polluting contributor graph with upstream history
3. Link via pnpm workspace: `"@comfyui/primevue": "workspace:*"`
4. Mark vendored directory in `.gitattributes` with `linguist-vendored`
Copy link
Contributor Author

@christian-byrne christian-byrne Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, point 4 doesn't really make sense since we will be making adjustments to the vendored library over time.

@christian-byrne christian-byrne added the documentation Improvements or additions to documentation label Aug 27, 2025
AustinMroz
AustinMroz previously approved these changes Aug 27, 2025
@DrJKL DrJKL self-assigned this Aug 27, 2025
@DrJKL
Copy link
Contributor

DrJKL commented Aug 28, 2025

Discussed elsewhere: We might be able to use Nx import to do the setup.

@christian-byrne
Copy link
Contributor Author

Worried that using nx import will make future pulls from upstream primevue difficult relative to using git subtree

- Change status from Proposed to Rejected
- Document rationale: implementation complexity with dual monorepos,
  maintenance burden, alternative solutions available
- Add specific code citations and repository links
- Include alternative approach using shadcn/ui for selective replacement
- Rename 0003-fork-primevue-ui-library.md to 0004-fork-primevue-ui-library.md
- Update ADR number in document header from 3 to 4
- Update README.md index to include both CRDT ADR-0003 and PrimeVue ADR-0004
- Maintain chronological order and avoid numbering conflicts

## Status

Rejected
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to reflect final decision ("Rejected").

@christian-byrne christian-byrne assigned DrJKL and unassigned DrJKL Sep 13, 2025
@christian-byrne christian-byrne added the PrimeVue Related to upstream PrimeVue library label Sep 13, 2025
@christian-byrne christian-byrne merged commit cf093d0 into main Sep 13, 2025
24 of 25 checks passed
@christian-byrne christian-byrne deleted the c_byrne/adr/primevue-fork branch September 13, 2025 06:06
Myestery pushed a commit that referenced this pull request Sep 17, 2025
* ADR: Add PrimeVue fork decision record

Adds ADR-0003 documenting the decision to fork PrimeVue as a monorepo workspace package. Key rationale includes transform coordinate system conflicts and virtual canvas scroll interference that require component-level modifications.

* ADR: Reject PrimeVue fork decision

- Change status from Proposed to Rejected
- Document rationale: implementation complexity with dual monorepos,
  maintenance burden, alternative solutions available
- Add specific code citations and repository links
- Include alternative approach using shadcn/ui for selective replacement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation PrimeVue Related to upstream PrimeVue library size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants