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

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

Closed
wants to merge 37 commits into from

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Jan 8, 2024

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.

@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2024

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 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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants