Skip to content

fix(ui5-table): remove circular dependency from table and row#9261

Merged
DonkeyCo merged 5 commits intomainfrom
fix-table-cd
Jun 19, 2024
Merged

fix(ui5-table): remove circular dependency from table and row#9261
DonkeyCo merged 5 commits intomainfrom
fix-table-cd

Conversation

@DonkeyCo
Copy link
Copy Markdown
Contributor

  • remove cyclic import in TableRowBase of Table.ts
  • add TableUtils.ts, which adds duck-typed instanceof checks for safe imports

Fixes: #9247

DonkeyCo added 4 commits June 19, 2024 14:40
- remove cyclic import in TableRowBase of Table.ts
- add TableUtils.ts, which adds duck-typed instanceof checks for safe imports
- remove cyclic import in TableRowBase of Table.ts
- add TableUtils.ts, which adds duck-typed instanceof checks for safe imports
@DonkeyCo DonkeyCo assigned DonkeyCo and unassigned DonkeyCo Jun 19, 2024
return this;
}

isTableCellBase() {
Copy link
Copy Markdown
Contributor

@aborjinik aborjinik Jun 19, 2024

Choose a reason for hiding this comment

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

I do not get this idea and what is wrong with instanceof check? a very fundamental feature is taken from our hands and this is ok?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nnaydenow linked me this PR: #8822 referencing point 4.

Seems like the main issue is scoping, when you have multiple different instances of the framework in your application, instanceof is not that reliable anymore?

@DonkeyCo DonkeyCo requested a review from aborjinik June 19, 2024 14:58
@DonkeyCo DonkeyCo requested a review from aborjinik June 19, 2024 15:37
@DonkeyCo DonkeyCo merged commit 9932adf into main Jun 19, 2024
@DonkeyCo DonkeyCo deleted the fix-table-cd branch June 19, 2024 16:49
Copy link
Copy Markdown
Contributor

@aborjinik aborjinik left a comment

Choose a reason for hiding this comment

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

How about setting an attribute in the onEnterDom of base classes and check this attribute to determine?
For the Table this should not be necessary since the framework does this anyway.

@DonkeyCo
Copy link
Copy Markdown
Contributor Author

DonkeyCo commented Jun 20, 2024

@aborjinik We can certainly try that. It would be different to how other components would handle this, but I think the Table structure and its components is anyways pretty different to the existing components.

I'll try to implement the change in this PR: #9205

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.

[Table]: cyclic import between TableRow and Table

3 participants