Skip to content

dbeaver/pro#10177 pass defaults to main properties - #4538

Open
devnaumov wants to merge 1 commit into
develfrom
dbeaver/pro#10177-main-properties
Open

dbeaver/pro#10177 pass defaults to main properties#4538
devnaumov wants to merge 1 commit into
develfrom
dbeaver/pro#10177-main-properties

Conversation

@devnaumov

Copy link
Copy Markdown
Member

closes 10177

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 5 complexity

Metric Results
Complexity 5

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

}

if (config.driverId) {
const driver = await this.dbDriverResource.load(config.driverId, ['includeMainProperties']);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

includes is pattern we refused to support a while ago. it would be nice to create new resource DBDriverMainPropertiesResource and reuse it here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not scope of the ticket, code is already exist in this part

if (config.driverId) {
const driver = await this.dbDriverResource.load(config.driverId, ['includeMainProperties']);

if (config.mainPropertyValues) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we get config defaultStateGetter
in this helper mainPropertyValues is always an object

so this check is always true
probably there is a miss-match with the zod type here:

mainPropertyValues: schema.record(schema.string(), schema.any()).optional(),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, types are from zod schema, there mainPropertyValues is optional

// Don't call load in create mode. There we rely on the defaults set here, and useAutoLoad will load the part on mount.
if (this.formState.mode === FormMode.Edit) {
await this.optionsPart?.load();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something is off here

Previously we waited for part being loaded and set initialState. And only then we went to setting driverId and all attached config to it. And it was pretty straightforward and imperative for both form modes: create and edit

Right now it smells like race condition. What if in create mode we resolve setDriverId() before the optionsPart is actually loaded and set initialState?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What was the original issue here? Maybe we can resolve it differently?

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.

2 participants