-
Notifications
You must be signed in to change notification settings - Fork 10
VIDSOL-296: migrating to nx #276
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
VIDSOL-296: migrating to nx #276
Conversation
| if: steps.check-skip-ci.outputs.result == 'false' | ||
| run: | | ||
| yarn test | ||
| NX_DAEMON=false yarn test |
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.
Disable Nx caching for CI/CD. The pipelines always run in fully isolated, fresh installations of the app, so caching provides no benefit.
| # Exit on error and unset variables | ||
| set -eu | ||
|
|
||
| # skip on CI or if deps aren't installed yet |
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.
This change prevents Husky from running in CI or when dependencies are not installed.
0900c9a to
0d1b262
Compare
| "description": "Express-based backend with authenticated routes.", | ||
| "main": "index.js", | ||
| "type": "module", | ||
| "scripts": { |
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.
scripts are moved to project.json
| }, | ||
| "author": "", | ||
| "license": "MIT", | ||
| "dependencies": { |
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.
dependencies are now centralize... only one version of each package for the whole project.
| { | ||
| "extends": "../tsconfig.base.json", | ||
| "compilerOptions": { | ||
| /* Visit https://aka.ms/tsconfig to read more about this file */ |
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.
all comments where removed along with redundant keys which are already present on the tsconfig.base.json
c2b25a4 to
1cee957
Compare
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.
Pull Request Overview
This PR migrates the project from a Yarn workspaces structure to a formal Nx monorepo architecture to support future modularity and scalability. The migration consolidates dependency management, introduces Nx tooling for build orchestration, and establishes a library structure (libs/ui) while preserving existing application behavior.
Key Changes:
- Replaces workspace-based scripts with Nx executors and targets across
backend,frontend, andintegration-tests - Introduces centralized configuration (
nx.json,tsconfig.base.json,eslint.config.mjs) - Adds a new
libs/uilibrary scaffolded for shared UI components
Reviewed Changes
Copilot reviewed 107 out of 116 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Migrates scripts to Nx commands; hoists all dependencies to root; removes workspace declarations |
nx.json |
Defines Nx workspace configuration with plugins, target defaults, and build orchestration |
tsconfig.base.json |
Establishes shared TypeScript compiler options for all projects |
eslint.config.mjs |
Replaces .eslintrc with flat ESLint config supporting Nx and TypeScript type-checked rules |
frontend/, backend/, integration-tests/ |
Adds project.json for Nx targets; updates tsconfig.json to extend base config; migrates ESLint to flat config |
libs/ui/ |
Scaffolds new shared UI library with Vite, Vitest, and React configuration |
vitest.workspace.ts |
Configures Vitest workspace to discover test configurations across projects |
| Various source files | Removes obsolete ESLint disable comments; improves type annotations; fixes minor linting issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
01a2789 to
70d3f55
Compare
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.
Pull Request Overview
Copilot reviewed 111 out of 124 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
docs/DEPENDENCIES.md:1
- The dependency changed from '@mui/base' to '@base-ui-components/react' but the description still refers to 'MUI Base'. The description should be updated to accurately reflect the new package, or if this is an intentional migration from MUI Base, it should be noted.
# Frontend Dependencies
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
frontend/src/components/MeetingRoom/DeviceSettingsMenu/DeviceSettingsMenu.tsx
Outdated
Show resolved
Hide resolved
cbcf650 to
a6e0e67
Compare
7dfce0f to
f264925
Compare
OscarFava
left a comment
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.
LGTM! Good to have NX working!
| - [@emotion/react](https://www.npmjs.com/package/@emotion/react) > Simple styling in React. | ||
| - [@emotion/styled](https://www.npmjs.com/package/@emotion/styled) styled API for emotion | ||
| - [@mui/base](https://www.npmjs.com/package/@mui/base) MUI Base is a library of headless ('unstyled') React components and low-level hooks. You gain complete control over your app's CSS and accessibility features. | ||
| - [@base-ui-components/react](https://www.npmjs.com/package/@base-ui-components/react) MUI Base is a library of headless ('unstyled') React components and low-level hooks. You gain complete control over your app's CSS and accessibility features. |
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.
mui/base is deprecated we changed to base-ui-components/react]
| // #endregion | ||
|
|
||
| // React | ||
| 'react/react-in-jsx-scope': 'off', |
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.
react 19 doesn't need react on the scope
|
|
||
| // test will fail without the await act | ||
| // eslint-disable-next-line @typescript-eslint/await-thenable | ||
| await act(() => { |
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.
this await is basically a await Promise.resolve() or... wait until next tick
behei-vonage
left a comment
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.
LGTM! can you please make sure that README.md is up-to-date in a follow up PR? thanks 🙏
|
sorry, just noticed there is lots of conflicts |
We better fix README now! Good catch! |
c22dfd3
c22dfd3 to
5d78639
Compare
VZaphod
left a comment
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.
LGTM
56ee9e4 to
21304cd
Compare
21304cd to
e3c904b
Compare
|
Hi I added a note that the project uses NX... but for the rest, scripts are the same, project structure is the same, and final dependencies are the same too. |
VZaphod
left a comment
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.
LGTM
| * @returns {GetLayout} getLayout | ||
| */ | ||
| const useLayoutManager = (): GetLayout => { | ||
| const layoutManager = useRef(new LayoutManager()); |
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.
useRef(new LayoutManager())The old code was creating in ever render a new instance of the LayoutManager, the subsequent instances were just discarded because of how useRef actually works but still was a bad implementation.
The new version crease an unique instance, only once and exports only the bounded getLayout function
OscarFava
left a comment
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.
Great job!!
rserebrennykov
left a comment
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.
LGTM



What is this PR doing?
This PR migrates the project from a Yarn workspaces setup into a formal monorepo architecture.
The goal is to establish the foundational structure required for future no-code and low-code capabilities by enabling better modularity, component isolation, and long-term scalability.
The migration introduces centralized dependency management, improved cross-package collaboration, and a more maintainable structure without changing the existing app layout or behavior.
How should this be manually tested?
What are the relevant tickets?
A maintainer will add this ticket number.
Resolves VIDSOL-
Checklist
[ ] Branch is based on
develop(notmain).[ ] Resolves a
Known Issue.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md?[ ] Resolves an item reported in
Issues.If yes, which issue? Issue Number?