[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

refactor!: update to use dynamic imports for web components #210

Closed
wants to merge 37 commits into from
Closed

refactor!: update to use dynamic imports for web components #210

wants to merge 37 commits into from

Conversation

web-padawan
Copy link
Member
@web-padawan web-padawan commented Jan 8, 2024 •
edited
Loading

Description

Related to #199

  • By default, web components are only imported dynamically on the first React components render,
  • React components also provide define() helpers to force loading web components on demand.

Type of change

  • Breaking change

@web-padawan web-padawan force-pushed the wip/dynamic-import branch 4 times, most recently from 81915fb to cb4a97a Compare January 10, 2024 11:52
@web-padawan
Copy link
Member Author

Tests for grid column fail with the following error related to recalculating column width:

TypeError: Cannot read properties of undefined (reading 'filter')
    at Grid.__setVisibleCellContentAutoWidth (chunk-IQKOLFSK.js?v=61d41aa2:4773:19)
    at chunk-IQKOLFSK.js?v=61d41aa2:4813:32
    at Array.forEach (<anonymous>)
    at Grid.__calculateAndCacheIntrinsicWidths (chunk-IQKOLFSK.js?v=61d41aa2:4813:10)
    at Grid._recalculateColumnWidths (chunk-IQKOLFSK.js?v=61d41aa2:4759:10)
    at Grid.recalculateColumnWidths (chunk-IQKOLFSK.js?v=61d41aa2:4832:10)
    at Grid._columnTreeChanged (chunk-IQKOLFSK.js?v=61d41aa2:5087:10)
    at Object.runMethodEffect [as fn] (chunk-H7ZGI7AV.js?v=61d41aa2:2238:38)
    at runEffects (chunk-H7ZGI7AV.js?v=61d41aa2:1782:16)
    at Grid._propertiesChanged (chunk-H7ZGI7AV.js?v=61d41aa2:2873:7)

Apparently, because of dynamic imports for custom columns, _columnTreeChanged is invoked before column element is upgraded, so column._allCells is still undefined here and here. I will try to reproduce this in the web component.

@web-padawan web-padawan mentioned this pull request Jan 10, 2024
Closed
@web-padawan web-padawan marked this pull request as ready for review January 15, 2024 11:26
@codecov-commenter
Copy link
codecov-commenter commented Jan 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (afe09ca) 95.71% compared to head (84e7dc2) 97.39%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
+ Coverage   95.71%   97.39%   +1.67%     
==========================================
  Files          30       30              
  Lines         280      345      +65     
  Branches       34       36       +2     
==========================================
+ Hits          268      336      +68     
  Misses          2        2              
+ Partials       10        7       -3
Flag Coverage Δ
unittests 97.39% <100.00%> (+1.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@web-padawan web-padawan mentioned this pull request Jan 16, 2024
Merged
@web-padawan web-padawan force-pushed the wip/dynamic-import branch 11 times, most recently from 2dcc352 to 6f244ee Compare January 17, 2024 08:16
@web-padawan
Suppress post-finalize style warning
6948a00
@web-padawan
Revert GridDefaultItem import change
9652fb1
@web-padawan
Make Notification.show import the WC if needed
d90f5d8
@web-padawan
Mark CrudEditColumn as element without theme
d4b6ad1
@web-padawan
Add vaadin-crud-edit to elements without theme
3fbf642
@web-padawan
Apply code review suggestion regarding render
e4533c2
@web-padawan
Add define option to call importFunc explicitly
8649529
@web-padawan
Update Dialog to expose define helper
af6f700
@web-padawan
Fix MenuBar test suite to use type-only import
c77b8ca
@web-padawan
Update ContextMenu to expose define helper
acfa5ba
@web-padawan
Update Select to expose define helper
16e76c0
@web-padawan
Update VirtualList to expose define helper
bf4b085
@web-padawan
Update ComboBox to expose define helper
2d36906
@web-padawan
Update ComboBoxLight to expose define helper
286ec7f
@web-padawan
Update MenuBar to expose define helper
b862067
@web-padawan
Update MultiSelectComboBox to expose define helper
5089f60
@web-padawan web-padawan force-pushed the wip/dynamic-import branch 2 times, most recently from 4722b9a to ddf7d3a Compare January 17, 2024 10:05
tomivirkki
tomivirkki previously approved these changes Jan 17, 2024
View reviewed changes
scripts/generator.ts Outdated Show resolved Hide resolved
scripts/generator.ts Outdated Show resolved Hide resolved
@web-padawan
Copy link
Member Author

Based on the internal discussion, we decided to proceed with #221 without adding dynamic imports. Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomivirkki tomivirkki tomivirkki left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@web-padawan @codecov-commenter @tomivirkki