-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix: dataset modal doesn't work in dev mode #18658
Conversation
@@ -344,6 +344,11 @@ const config = { | |||
], | |||
use: [babelLoader], | |||
}, | |||
{ |
There was a problem hiding this comment.
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.
{ | ||
test: /\.js$/, | ||
include: /node_modules\/react-dom/, | ||
use: ['react-hot-loader/webpack'], | ||
}, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh, good point! 😓
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
And we use
recurseReactClone
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 passonChange
props. see screenshot: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