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

In React, the Instant field of the generated model is of string type. #23769

Closed
1 task done
hide212131 opened this issue Oct 8, 2023 · 12 comments · Fixed by #24367
Closed
1 task done

In React, the Instant field of the generated model is of string type. #23769

hide212131 opened this issue Oct 8, 2023 · 12 comments · Fixed by #24367
Labels
area: enhancement 🔧 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: react $100 https://www.jhipster.tech/bug-bounties/
Milestone

Comments

@hide212131
Copy link
Contributor

Overview of the issue

JDL file:

entity Employee {
        hireDate Instant,
}

TypeScript model: (In Angular, it's of Dayjs type, while in Vue, it's of Date type.)

export interface IEmployee {
  hireDate?: string | null;
}

During the entity update process, a Date type value is stored in the model.

  const saveEntity = values => {
    // type of convertDateTimeToServer is `(date: any) => Date`
    values.hireDate = convertDateTimeToServer(values.hireDate);
Motivation for or Use Case

While this doesn't directly cause an error, it can lead to confusion during frontend development.

Reproduce the error

Generate React code using a JDL file that includes an Instant type field.

Related issues
Suggest a Fix

Change it to the Date type. In the long run,
it would be beneficial to reduce the usage of the any type and enhance type safety.

JHipster Version(s)

7.9.4, 8.0.0-beta3

JHipster configuration, a .yo-rc.json file generated in the root folder
.yo-rc.json file
{
  "generator-jhipster": {
    "applicationType": "monolith",
    "authenticationType": "jwt",
    "baseName": "myApp",
    "buildTool": "maven",
    "cacheProvider": "ehcache",
    "clientFramework": "react",
    "clientTestFrameworks": [],
    "clientTheme": "none",
    "creationTimestamp": 1696591837279,
    "databaseType": "sql",
    "devDatabaseType": "h2Disk",
    "devServerPort": 9060,
    "enableGradleEnterprise": null,
    "enableHibernateCache": true,
    "enableSwaggerCodegen": false,
    "enableTranslation": true,
    "entities": [
      "Employee"
    ],
    "gradleEnterpriseHost": null,
    "jhipsterVersion": "8.0.0-beta.3",
    "languages": [
      "ja",
      "en",
      "fr"
    ],
    "lastLiquibaseTimestamp": 1696591897000,
    "messageBroker": false,
    "microfrontend": null,
    "microfrontends": [],
    "nativeLanguage": "ja",
    "packageName": "com.mycompany.myapp",
    "prodDatabaseType": "postgresql",
    "reactive": false,
    "searchEngine": false,
    "serverPort": null,
    "serverSideOptions": [],
    "serviceDiscoveryType": false,
    "testFrameworks": [],
    "websocket": false,
    "withAdminUi": true
  }
}
Environment and Tools

openjdk version "17.0.8" 2023-07-18
OpenJDK Runtime Environment GraalVM CE 22.3.3 (build 17.0.8+7-jvmci-22.3-b22)
OpenJDK 64-Bit Server VM GraalVM CE 22.3.3 (build 17.0.8+7-jvmci-22.3-b22, mixed mode, sharing)

git version 2.39.3 (Apple Git-145)

node: v18.17.1
npm: 9.6.7

Docker version 24.0.6, build ed223bc

JDL for the Entity configuration(s) entityName.json files generated in the .jhipster directory
JDL entity definitions
entity Employee {
  firstName String
  salary Long
  hireDate Instant
}
search Employee with no

Browsers and Operating System
  • Checking this box is mandatory (this is just to show you read everything)
@hide212131 hide212131 changed the title In React, the date field of the generated model is of string type. In React, the Instant field of the generated model is of string type. Oct 8, 2023
@hide212131
Copy link
Contributor Author

It looks like the type was changed to string in this PR, whereas it used to be a Moment type. Currently, it's dayjs, and when reverted, no issues arise in my local environment (running npm test in the react-default app).
Reverting will result in a dayjs type, but...

export const convertDateTimeToServer = date => date ? dayjs(date).toDate() : null;

The utility function convertDateTimeToServer returns a Date type, not a dayjs type.
I believe it would be better to modify the function to return a dayjs object.

If this direction is acceptable, we will issue a PR.

@hide212131
Copy link
Contributor Author

hide212131 commented Oct 17, 2023

I think a AxiosResponseTransformer is needed when storing entity information from an axios response.

@DanielFran
Copy link
Member

@qmonmert Can you take a look at this one too?

@qmonmert
Copy link
Contributor

@DanielFran I think we can't change that!

image
Indeed input for Instant is datetime-local and the doc https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/datetime-local explains that the value is of type string
image

@hide212131
Copy link
Contributor Author

@qmonmert @DanielFran, thank you for progressing with this issue. I'm glad.

Yes, it is true that the input type for Date is string.
The issue I'm mentioning relates to the 'Model' as shown below:
Form(string) -> Model(Date or dayjs) -> to server

Below is a sample code snippet that adds a type to the generated code and results in an error:

export interface IEmployee {
  hireDate?: string | null;
}

// Convert date (input form `datetime-local`, value is of type `string`) to Date type (for the server)
export const convertDateTimeToServer = (date?: string) : Date | null => (date ? dayjs(date).toDate() : null);

const saveEntity = (formValues: Record<string, any>) => {    
    const employeeModel: IEmployee = {};
    employeeModel.hireDate = convertDateTimeToServer(formValues.hireDate as string); // ERROR: Type 'Date' is not assignable to type 'string'.ts(2322)
};

My suggestion is as follows, involving changing the dayjs type and adding type annotations in this fix:

export interface IEmployee {
  hireDate?: dayjs.Dayjs | null;
}

export const convertDateTimeToServer = (date?: string) : dayjs.Dayjs | null => (date ? dayjs(date) : null);

@qmonmert
Copy link
Contributor

@hide212131 PR done to try this :)

@hide212131
Copy link
Contributor Author

@qmonmert Thank you very much!
I would like to discuss further here on this issue. The reasons are as follows:

  1. I think the changes in the PR are almost fine in the Client -> Server direction. (A little modification is needed, which I will mention later.)
  2. I believe some modifications are necessary in the Server -> Client direction.

Below, I have created an example of the code generation I want to achieve. Could you please take a look at it? It's code for sending and receiving an Employee model with a hireDate.
hide212131/jhipster-sample-app@801cd2f

This was written based on Angular code generation. The implementation in this code includes:

Overall

  • To clarify REST data, I defined RestEmployee, which is composed only of string type.

Client -> Server

  • Structure: reactForm (form with datetime-local string type) -> Model (IEmployee with dayjs type) -> RestData (RestEmployee with ISO format string type)
  • By making hireDate a dayjs type, it's necessary to convert it to an ISO format string for REST. In practice, passing dayjs directly to axios yields the same result, but for type safety and explicit conversion, I have written the code.

Server -> Client

  • Structure: RestData (RestEmployee with ISO format string type) -> Model (IEmployee with dayjs type) -> reactForm (form with datetime-local string type)
  • By making hireDate a dayjs type, it's necessary to convert the ISO format string obtained from REST into dayjs type. In the code, this conversion is done in createEntitySlice (Ideally, it should be done in createAsyncThunk, but currently, the code in createEntitySlice is the first to handle the contents of AxiosResponse)

Reference

In Angular, similar functionality is achieved with the following code. (For TypeSafe code, Angular's code can be a reference.)

  • Here, realizing Form <-> Model conversion.
  • Here, realizing Model <-> RestData conversion.

As I researched, I realized that it's not as simple a fix as I initially thought.
I am also willing to help as much as possible.

@DanielFran DanielFran added $100 https://www.jhipster.tech/bug-bounties/ $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ labels Nov 30, 2023
@DanielFran
Copy link
Member

@qmonmert Adding a bug bounty

@qmonmert
Copy link
Contributor

@hide212131 there is a bug with the actual PR? Because I don't know if your proposal is necessary :)

@hide212131
Copy link
Contributor Author

@qmonmert There is no 'bug' with the current PR. I have conducted a verification. The behavior remains unchanged from before, and it is safe to proceed with the merge.

The next issue I would like to resolve is the following event.

export interface IEmployee {
  hireDate?: dayjs.Dayjs | null;
}

export const EmployeeUpdate = () => {
  const employeeEntity = useAppSelector<IEmployee>(state => state.employee.entity);

  // If we write this expecting hireDate to be a dayjs type, we get an error at runtime. 
  // This is because hireDate can be a **string** type.
  const year = employeeEntity.hireDate?.year(); 

@qmonmert
Copy link
Contributor

qmonmert commented Dec 1, 2023

@DanielFran
Copy link
Member

@qmonmert approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: enhancement 🔧 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: react $100 https://www.jhipster.tech/bug-bounties/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants