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

[REFACTOR] Migrating workflow common types to twenty-shared #10048

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Feb 5, 2025

Introduction

WIP

This PR result from a discussion in #10036
This is a refactor of the workflow types extraction to twenty-shared refactor.

In a nutshell instead of importing workflow types like:

import { SomeWorkflowType } from "twenty-shared";

We now expose a scoped barrel as follows:

import { SomeWorkflowType } from "twenty-shared/workflow";

Vscode import suggestion

From the moment you've added in your vscode project at least one import to a specific barrel which is not your package default entry ( twenty-shared). Then vscode will start suggesting imports from this barrel :)

// @filename some-file.ts
import { SomeWorkflowTypeA } from "twenty-shared/workflow";

// @filename other-file.ts
import { SomeWorkflowTypeA } from "twenty-shared/workflow"; // addMissingImports also auto-importing on save !

const whatever: SomeWorkflowTypeB = 'never' // cmd + esc or ctr + space_bar suggests import from `twenty-shared/worfklow`

IMO Several scoped barrel for a better sleep and maintainability ( and enhance tree shaking ? To attest )

Diff from #10036

  • removed .type extension file
  • renamed a missleading file
  • added a @/ leading to ./src/* paths within tsconfig.json to avoid asserting baseUrl is defined
  • updated vite roll-up and package config

Dispatching the extract AKA updating imports

Could not find a way to reproduce identication done in #10036, lets discuss this
Also I feel like naming an grouping should be reviewed

Codegen

We should programmatically handle and maintain or file structure and import along twenty-ui and twenty-shared such as does the generateBarrel from twenty-ui but with multiple barrel support. I will be working on this on my next slack day.

ps: As a cli and within its own package etc could be smoother

Copy link

github-actions bot commented Feb 5, 2025

Warnings
⚠️

Changes were made to package.json, but not to yarn.lock - Perhaps you need to run yarn install?

TODOs/FIXMEs:

  • // TODO Centralize and sync with package.json programmatically: packages/twenty-shared/vite.config.ts

Generated by 🚫 dangerJS against 9de4de8

@@ -0,0 +1,4 @@
export * from './toRename';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Request changes: Please find an accurate name for this, was worklfow/Workflow/ before, temporarily swapped it to toRename

packages/twenty-shared/src/index.ts Outdated Show resolved Hide resolved
@FelixMalfait
Copy link
Member

Cool to make progress on this! When should we rely on GraphQL:generate codegen vs this to share types? It's hard for me to draw a clear line. I feel like all could have gone through the GraphQL API?

We need to think about the experience for someone building an app in the future, how do apps interact etc. I think I prefered having the server as a source of truth as much as possible but probably I'm lacking context.

@lucasbordeau
Copy link
Contributor

lucasbordeau commented Feb 6, 2025

I agree with @FelixMalfait we should maybe find a way to derive everything from GraphQL, especially enums.

Although it could be possible to identify enums which could have their source of truth in twenty-shared, then used in our entities in the backend, most of them should probably be used from our generated types on the frontend.

The rule of thumb here should be to find where the source of truth should be for each type, which can be different from only GraphQL.

Also how do we handle generated types with __typename or that do not export some types to the frontend because it shouldn't know about some backend only props ?

Because of TypeScript duck typing, we can tolerate having two source of truth if the types match their properties but we should agree on a convention to easily determine one source of truth per type.

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

Successfully merging this pull request may close these issues.

3 participants