Skip to content
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

[MONOREPO] Twenty-ui as a package #9941

Closed
wants to merge 21 commits into from
Closed

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Jan 30, 2025

Introduction

In this PR we've migrated from a TypeScriptproject dependency using custom paths to a local package dependency, now consuming the built version of twenty-ui and added it as an external third party dependency provisioned by yarn workspaces

Encountered a bug between built and code types

When compiling your TypeScript if you use custom module import paths, they will appear in built project too.
But won't be resolvable.
Enters in scene third party library solutions such as resolve-alias entry in vite config ( requires manual configuration) or in our case vite-tsconfig-paths

When you don't resolve custom imports in your compiled TypeScript project it might have side effect on dependent project importing your type. They will might infer types from incorrectly fixed by TypeScript types ( from my understanding ). Which can mutate some field from optional to required, bug we encountered on the IconComponent

Closes twentyhq/core-team-issues#281

@@ -1,5 +1,11 @@
{
"extends": "./tsconfig.json",
"compilerOptions": {
"declaration": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context: same motivations as for #9838

@prastoin prastoin marked this pull request as ready for review January 30, 2025 23:51
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR migrates the project from TypeScript path aliases to using twenty-ui as a proper package dependency, focusing on improving module resolution and type inference.

  • Removes custom path mappings (@ui/*) from tsconfig files and replaces them with direct imports from twenty-ui package
  • Adds baseUrl configuration in build configs to ensure proper module resolution
  • Updates import statements across multiple state management files to use twenty-ui package instead of local paths
  • Fixes potential type inference issues where optional fields could be incorrectly marked as required in built packages
  • Configures twenty-ui package with declaration file generation and source maps for better type support

108 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -1,4 +1,4 @@
import { createState } from '@ui/utilities/state/utils/createState';
import { createState } from 'twenty-ui';

export const isCurrentUserLoadedState = createState<boolean>({
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The file name in the export ('isCurrentUserLoadedState') doesn't match the file name ('isCurrentUserLoadingState.ts'). This inconsistency should be addressed.

@@ -1,4 +1,4 @@
import { createState } from '@ui/utilities/state/utils/createState';
import { createState } from 'twenty-ui';

export const viewableRecordIdState = createState<string | null>({
key: 'activities/viewable-record-id',
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The key 'activities/viewable-record-id' seems misplaced since this file is in the object-record module, not activities. Should be 'object-record/viewable-record-id'.

Comment on lines +4 to +5
"declaration": true,
"declarationMap": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Enabling both declaration and declarationMap will generate larger build artifacts. Consider if source maps are really needed for declaration files in production builds.

@prastoin prastoin marked this pull request as draft January 31, 2025 12:31
@prastoin
Copy link
Contributor Author

Started a biggest refactor to eject from one for all barrel on the twenty-ui package a bit earlier than planned because encountered breaking changes while importing the whole barrel in jest tests and so on

Will extract the barrel ejection migration into a scoped PR for the moment POCing here

Copy link

github-actions bot commented Jan 31, 2025

TODOs/FIXMEs:

  • // TODO outsource the URL computation, should not be done within twenty-ui: packages/twenty-ui/src/display/avatar/components/Avatar.tsx
  • // TODO CHECK IF REMOVEABLE: packages/twenty-ui/vite.config.ts

Generated by 🚫 dangerJS against b3526f7

@prastoin
Copy link
Contributor Author

The upcoming pain point that will be updating the 1k imports from twenty-ui in the twenty-front ~

@prastoin
Copy link
Contributor Author

@greptileai trigger

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This PR continues the migration from TypeScript path aliases to package-based dependencies, focusing on UI components and their dependencies.

  • Added required baseUrl prop to Avatar components across stories and implementations to fix server URL handling
  • Migrated icon imports from internal @ui/display to @tabler/icons-react for better package independence
  • Removed barrel files (index.ts) across multiple directories to enforce explicit imports
  • Fixed import paths in button components to use direct paths instead of aliases (e.g., @ui/display/icon/types/IconComponent)
  • Removed generateBarrels.js script and related configuration as part of eliminating barrel exports

The changes maintain functionality while improving module resolution and type inference. The removal of barrel files and addition of explicit imports should make dependencies clearer and prevent potential circular dependencies.

107 file(s) reviewed, 21 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 132 to 134
optimizeDeps: {
exclude: ['../../node_modules/.vite', '../../node_modules/.cache'],
exclude: ['../../node_modules/.vite/*', '../../node_modules/.cache/*'],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The wildcard pattern '../../node_modules/.vite/*' might not catch all subdirectories. Consider using '../../node_modules/.vite/**' for recursive matching.

@@ -8,7 +8,7 @@ const jestConfig: JestConfigWithTsJest = {
preset: '../../jest.preset.js',
setupFilesAfterEnv: ['./setupTests.ts'],
testEnvironment: 'jsdom',
transformIgnorePatterns: ['../../node_modules/'],
transformIgnorePatterns: ['../../node_modules/', "./dist/"],
Copy link
Contributor

Choose a reason for hiding this comment

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

style: inconsistent quote style used for paths - './dist/' uses double quotes while '../../node_modules/' uses single quotes

Comment on lines 6 to 25
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.js",
"require": "./dist/index.cjs"
"./*": {
"import": {
"types": "./dist/*.d.ts",
"default": "./dist/*.js"
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: This exports configuration may cause issues with CommonJS environments since the 'require' export was removed. Consider keeping both ESM and CommonJS exports if backward compatibility is needed.

Comment on lines +7 to +23
"./*": {
"import": {
"types": "./dist/*.d.ts",
"default": "./dist/*.js"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The wildcard pattern './' with dist/.d.ts could potentially expose internal implementation details. Consider being more explicit about which files can be imported.

@@ -0,0 +1 @@
export const SERVER_URL_MOCK = 'http://localhost:3000'
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using an environment variable instead of a hardcoded URL to make the mock URL configurable across different environments

packages/twenty-ui/src/utilities/config/index.ts Outdated Show resolved Hide resolved
Comment on lines +65 to +68
copyPublicDir: false,
minify: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: disabling minification in production build could impact performance and bundle size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tmp

Comment on lines 85 to 90
.sync('src/**/*.{ts,tsx}', {
ignore: ['src/**/*.d.ts'],
})
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider also ignoring test files (*.test.ts, .spec.ts) and story files (.stories.tsx) to avoid including them in the build

Suggested change
.sync('src/**/*.{ts,tsx}', {
ignore: ['src/**/*.d.ts'],
})
.sync('src/**/*.{ts,tsx}', {
ignore: ['src/**/*.d.ts', 'src/**/*.test.{ts,tsx}', 'src/**/*.spec.{ts,tsx}', 'src/**/*.stories.{ts,tsx}'],
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gooood booot, exactly what I couldnt' see yesterday !

charlesBochet pushed a commit that referenced this pull request Feb 1, 2025
…ared` (#9967)

# Introduction
Avoid having multiple `isDefined` definition across our pacakges
Also avoid importing `isDefined` from `twenty-ui` which exposes a huge
barrel for a such little util function

## In a nutshell
Removed own `isDefined.ts` definition from `twenty-ui` `twenty-front`
and `twenty-server` to move it to `twenty-shared`.
Updated imports for each packages, and added explicit dependencies to
`twenty-shared` if not already in place

Related PR #9941
@prastoin prastoin force-pushed the twenty-ui-as-package-2 branch from 5fcbcf7 to da03822 Compare February 1, 2025 15:12
@prastoin
Copy link
Contributor Author

prastoin commented Feb 4, 2025

Closing this to be reopened later

@prastoin prastoin closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Migrate twenty-ui as a package
1 participant