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

Future Admin Generator: parse config using ts-morph to support inline functions and direct imports #3297

Open
wants to merge 25 commits into
base: next
Choose a base branch
from

Conversation

nsams
Copy link
Member

@nsams nsams commented Jan 29, 2025

The Problem

Currently the .cometGen.ts config files are evaluated as code. This has one major drawback: we can't parse inline code, as it will be executed, or, if it's a function definition, we don't know if it's inline or imported.

The Proposed Solution

Instead we parse the config file using ts-morph and iterate the resulting AST to get a json structure similar to before, but with the flexibility to parse for imports or inline functions.

Example, inline function (validate)

config with inline function:

                {
                    type: "text",
                    name: "title",
                    label: "Titel", // default is generated from name (camelCaseToHumanReadable)
                    required: true, // default is inferred from gql schema
                    validate: (value: string) => (value.length < 3 ? "Title must be at least 3 characters long" : undefined),
                },

generated code:

                            <TextField
                                required
                                variant="horizontal"
                                fullWidth
                                name="title"
                                label={<FormattedMessage id="product.title" defaultMessage="Titel" />}
                                validate={(value: string) => (value.length < 3 ? "Title must be at least 3 characters long" : undefined)}
                            />

Example, imported block

import { DamImageBlock } from "@comet/cms-admin";
// ...
{ type: "block", name: "image", label: "Image", block: DamImageBlock },

Disadvantages

Any other "a bit" advanced definition of config have to be explicitly implemented, as the config is not evaluated anymore. This includes any factories, helpers etc. Problem is that we can't use typescript to forbid them.

For example, the following needs custom (ast "walking") code to be supported:

const typeValues = [{ value: "Cap", label: "great Cap" }, "Shirt", "Tie"];

export const ProductsGrid: GridConfig<GQLProduct> = {
    type: "grid",
    gqlType: "Product",
   ///...
    columns: [
        {
            type: "combination",
            name: "overview",
            headerName: "Overview",
            minWidth: 200,
            primaryText: "title",
            secondaryText: {
                type: "formattedMessage",
                message: "{price} • {type} • {category} • {inStock}",
                    type: {
                        type: "staticSelect",
                        field: "type",
                        values: typeValues, //<< this identifier references a value defined in this file. NOT supported anymore.
                        emptyValue: "No type",
                    },

For example this would not be supported:

const typeValues = [{ value: "Cap", label: "great Cap" }, "Shirt", "Tie"];
typeValues.push("Foo");

Breaking change

The old import is not supported anymore, change:

block: { name: "DamImageBlock", import: "@comet/cms-admin" },

to:

import { DamImageBlock } from "@comet/cms-admin";
//...
block: DamImageBlock },

TODO

  • implement missing json conversion parts
  • allow usage of tsx, for example to use <FormattedMessage in validate
  • use new way everywhere where we have imports
  • typing of code-generation-runtime vs. config-definition is incorrect

Task: https://vivid-planet.atlassian.net/browse/COM-1626

@auto-assign auto-assign bot requested a review from johnnyomair January 29, 2025 12:11
@@ -422,14 +421,14 @@ export function ProductForm({ id }: FormProps): React.ReactElement {
name="priceList"
label={<FormattedMessage id="product.priceList" defaultMessage="Price List" />}
variant="horizontal"
maxFileSize={4194304}
maxFileSize={1024 * 1024 * 4}
Copy link
Member Author

Choose a reason for hiding this comment

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

here we also can keep the non-evaluated version (good)

Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

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

I believe the advantages of this approach outweigh the disadvantages. I'm not sure if devs ever want to code in config files. However, it's confusing that the file is a TS file, but operations won't work...

Maybe we could find a way to use both ts-morph and execute the file? I'm think of mocking/replacing the imports with stubs, like jest.mock does.

@nsams nsams force-pushed the admingen-tsmorph branch 2 times, most recently from 2e53ac9 to 9848727 Compare February 14, 2025 15:03
@nsams nsams changed the base branch from main to next February 15, 2025 18:01
@nsams nsams changed the title Future Admin Generator PoC: parse config using ts-morph to support in… Future Admin Generator: parse config using ts-morph to support inline functions and direct imports Feb 20, 2025
@andrearutrecht andrearutrecht mentioned this pull request Feb 20, 2025
3 tasks
import { type GQLProductsGridFutureFragment } from "./future/generated/ProductsGrid.generated";
import { type GQLProductsListManualFragment } from "./ProductsGrid.generated";

type Props = GridCellParams<GQLProductsListManualFragment | GQLProductsGridFutureFragment>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change necessary? Was the usage incorrect?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the first param is the row type: https://github.com/mui/mui-x/blob/v7.x/packages/x-data-grid/src/models/params/gridCellParams.ts#L18, the second is the value.

(this was swapped in mui-x)

});
if ("import" in field.block) {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
imports.push(convertConfigImport(field.block as any)); // TODO: improve typing, generator runtime vs. config mismatch
Copy link
Collaborator

Choose a reason for hiding this comment

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

When do you plan to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an idea yet how this could be solved. We can talk about it today.

| SingleFileFormFieldConfig
| MultiFileFormFieldConfig
) & {
name: keyof T;
label?: string;
required?: boolean;
virtual?: boolean;
validate?: ImportReference;
validate?: (value: unknown) => ReactNode | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still support imports here? Might come in handy if we want to reuse the same validation for multiple forms.

Also, you can use the FieldValidator type here, which includes allValues and meta in the function declaration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we still support imports here?

yes, we do. It's just not visible anymore here in the types.

return tsSource;
}

const supportedImportPaths = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a good solution to the problem 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it is pretty simple. But it doesn't support recursions (eg. for fieldsets)

@thomasdax98 thomasdax98 added this to the v8.0.0 milestone Feb 26, 2025
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.

4 participants