TOSIT-IO / tdp-ui

Graphical interface for tdp-server
Apache License 2.0
4 stars 2 forks source link

Refactor to RTK Query #202

Closed sergkudinov closed 1 year ago

sergkudinov commented 1 year ago

Which issue(s) this PR fixes

Fixes #173 Fixes #112 Fixes #113 Fixes #114 Fixes #126 Fixes #168 Fixes #28

Replaces #201

Additional comments

It fully replaces the old way of querying the server and eliminates using OpenAPI generator on Java.

Agreements

sergkudinov commented 1 year ago

Just descovered a few issues:

Fixing it now

PaulFarault commented 1 year ago

We should add yarn.lock and package-lock.json to the .gitignore until the first release at least.

sergkudinov commented 1 year ago

added lock files to gitignore

PaulFarault commented 1 year ago

Looks promising!! RTK query definitely simplify the code.

If I'm not mistaken, it's missing:

Do you see anything else to add / do you need some help?


Also:

sergkudinov commented 1 year ago

@PaulFarault

Authentication

Done

Remove the need for a NEXT_PUBLIC_TDP_API_URL=http://localhost:8000 variable in the .env file

Done

We may want to refactor it a bit (code is spited between src/scripts/, src/features and src/store.ts). Maybe we could put all Redux related code in a src/store/ directory?

Agree, work in progress

I'm not sure that we should commit the /src/features/tdpApi.ts file as it is generated. We could generate it programmatically from the schema to ensure that the file is not altered.

Most likely, I'll check

PaulFarault commented 1 year ago

I've checked previously opened issues. Your PR will:

Fixes #112 Fixes #113 Fixes #114 Fixes #126 Fixes #168 Fixes #173 (current)

@sergkudinov you should add those on your PR description in order to close them automatically after merge.

sergkudinov commented 1 year ago

I'm not sure that we should commit the /src/features/tdpApi.ts file as it is generated. We could generate it programmatically from the schema to ensure that the file is not altered.

I tried the programmatic usage, but it doesn't work for in browser environment. Check this issue.

I think it is doesn't make a big sence commiting the genereated API in src/features/api/tdpApi.ts or not. We have a full control over the schema and it is stored in the same repository. Once it is changed, we regenerate the API and commit both. Git-ignoring it would just make the source code less dependent on instructions in the doc.

If we ignore, I would suggest:

Example:

    ...
    "generate": "npx @rtk-query/codegen-openapi ./scripts/openapi-config.ts",
    "postinstall": "npm run generate"
    ...
PaulFarault commented 1 year ago

I'm not sure to understand the issue but you're right, that's not very important. The solution that you propose using the postinstall script would be enought!

Idk if we could have another opinion but IMO we shouldn't commit the generated output.

sergkudinov commented 1 year ago

Done:

Working on:

sergkudinov commented 1 year ago

when switching between components on a service page, variables are not correctly changing in the Monaco edditor

Fixed this. However, swithing beteween components is not smooth anymore. Didn't have time to improve it. The was a problem of react rerendering when variables change.

There is now an issue with validating. It is not working as expected. I'll continue next week on fixing it.

sergkudinov commented 1 year ago

However, swithing beteween components is not smooth anymore.

Done

There is now an issue with validating. It is not working as expected.

Done

sergkudinov commented 1 year ago

@PaulFarault review please, it is ready

PaulFarault commented 1 year ago

I'll have a look at it. You still have some conflicts to resolve before merging it to master.

sergkudinov commented 1 year ago

rebased

PaulFarault commented 1 year ago

Seems like you involuntarily "broke" some previous features while rebasing, pagination doesn't seem to work as expected: https://github.com/TOSIT-IO/tdp-ui/pull/185

sergkudinov commented 1 year ago

Seems like you involuntarily "broke" some previous features while rebasing, pagination doesn't seem to work as expected: https://github.com/TOSIT-IO/tdp-ui/pull/185

fixed