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

[CHORE] Avoid isDefined duplicated reference, move it to twenty-shared #9967

Merged
merged 8 commits into from
Feb 1, 2025

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Feb 1, 2025

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 changed the title Move utils function into twenty shared [CHORE] Avoid isDefined duplicated reference, move it to twenty-shared Feb 1, 2025
@prastoin prastoin marked this pull request as ready for review February 1, 2025 09:52
@prastoin prastoin self-assigned this Feb 1, 2025
Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

LGTM!

@charlesBochet charlesBochet merged commit 7fd8967 into main Feb 1, 2025
52 checks passed
@charlesBochet charlesBochet deleted the move-utils-function-into-twenty-shared branch February 1, 2025 11:10
Copy link

github-actions bot commented Feb 1, 2025

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5591:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5562:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5571:7)�[39m
danger-results://tmp/danger-results-b9d65bb0.json

Generated by 🚫 dangerJS against ed1c1bf

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 consolidates the isDefined utility function by moving it from individual packages to twenty-shared, affecting multiple packages across the codebase.

  • Moves isDefined implementation from twenty-front, twenty-server, and twenty-ui to twenty-shared package to avoid duplication
  • Updates over 200 files to import isDefined from twenty-shared instead of local utilities or twenty-ui
  • Adds export of isDefined in twenty-shared/src/index.ts and moves test file to shared package
  • Removes isDefined.ts files from individual packages while maintaining identical functionality
  • Fixes incorrect variant/accent prop handling in ButtonGroup.tsx where additionalProps.variant was being set twice

559 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -17,7 +17,8 @@ import { useLingui } from '@lingui/react/macro';
import { OnSelectionChangeParams, useOnSelectionChange } from '@xyflow/react';
import { useCallback } from 'react';
import { useSetRecoilState } from 'recoil';
import { IconBolt, isDefined, useIcons } from 'twenty-ui';
import { isDefined } from 'twenty-shared';
import { IconBolt, useIcons } from 'twenty-ui';

export const WorkflowDiagramCanvasEditableEffect = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Missing import for useIcons which was previously imported from twenty-ui

Suggested change
export const WorkflowDiagramCanvasEditableEffect = () => {
import { useIcons } from '@/ui/utilities/icons/hooks/useIcons';
const { getIcon } = useIcons();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants