Skip to content

Conversation

stephenLYZ
Copy link
Member

@stephenLYZ stephenLYZ commented Feb 10, 2022

SUMMARY

This problem only exists in dev environments. The reason is the react-hot-loader will use "ProxyFacade", which is a wrapper for Functional Component.

ProxyFacade = function(props, context) {
      /*

        ! THIS IS NOT YOUR COMPONENT !
        !  THIS IS REACT-HOT-LOADER  !

        And you are probably looking for a function component of yours
        It's hidden, but there is a way to fix this - just reconfigure your application a bit
        see https://github.com/gaearon/react-hot-loader/issues/1311

       */

      const result = CurrentComponent(props, context);

And we userecurseReactClone method to create component(see https://github.com/apache/superset/blob/master/superset-frontend/src/CRUD/utils.js), which will pass props to the component. In this case, since the name has been changed, it is not possible to pass onChange props. see screenshot:

image

Here we use this solution. gaearon/react-hot-loader#1311

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

befre

dev-server.mp4

after

2022-02-10.5.17.06.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Dataset modal doesn't work correctly in dev server mode #18655
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@@ -344,6 +344,11 @@ const config = {
],
use: [babelLoader],
},
{
Copy link
Member

Choose a reason for hiding this comment

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

Please add a todo comment here and link to this PR. We should refactor DatasourceEditor.jsx and decouple it in future.

@zhaoyongjie zhaoyongjie self-requested a review February 10, 2022 09:39
Comment on lines +347 to +351
{
test: /\.js$/,
include: /node_modules\/react-dom/,
use: ['react-hot-loader/webpack'],
},
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should consider using @hot-loader/react-dom, as maybe there are other patches there, too, that we're just unaware of right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. However, this lib has not been updated for three years. 😂

Copy link
Member

Choose a reason for hiding this comment

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

Uhh, good point! 😓

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI. We may use react-refresh to replace react-hot-loader in the future, which is more active.

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this!

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #18658 (bdceb5a) into master (00eb6b1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #18658   +/-   ##
=======================================
  Coverage   66.29%   66.29%           
=======================================
  Files        1603     1603           
  Lines       62744    62744           
  Branches     6320     6320           
=======================================
  Hits        41593    41593           
  Misses      19499    19499           
  Partials     1652     1652           
Flag Coverage Δ
javascript 51.28% <ø> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00eb6b1...bdceb5a. Read the comment docs.

@villebro villebro merged commit 8212975 into apache:master Feb 10, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 First shipped in 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.5.0 First shipped in 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants