You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/18 18:47:07 UTC
[GitHub] [superset] yardz opened a new pull request #13220: Tests for flash provider
yardz opened a new pull request #13220:
URL: https://github.com/apache/superset/pull/13220
### SUMMARY
- Creation of a `componentDidMount` hook. The reason for this hook is so that there are fewer lint errors and there is no need to put ``// eslint-disable-next-line react-hooks/exhaustive-deps`` throughout the project
- Updated the ``useEffect(effect, [])`` to use the new hook (remove some lint errors and improve reading)
- Switched FlashProvider to function component and created tests for it (Also simplify the props he receives)
- I separated the creation of the Store from the App file. To facilitate the maintenance and creation of new reducers. And also to facilitate the testing of some components that need to perform dispatches.
Despite the change in the FlashProvider interface, no change in behavior should occur in the application.
### TEST PLAN
All tests must pass
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579522576
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I put it just thinking about avoiding repeated code. I reversed the commit and didn't apply it anywhere else.
I agree that the hook will be little used, but I am of the opinion that if it is used 2 times, then it should be isolated in a file and reused (it is better for testing, avoiding bugs, etc.).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13220:
URL: https://github.com/apache/superset/pull/13220#issuecomment-782544966
# [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=h1) Report
> Merging [#13220](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=desc) (efb4839) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `19.69%`.
> The diff coverage is `64.06%`.
[](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13220 +/- ##
===========================================
+ Coverage 53.06% 72.75% +19.69%
===========================================
Files 489 558 +69
Lines 17314 20545 +3231
Branches 4482 5378 +896
===========================================
+ Hits 9187 14947 +5760
+ Misses 8127 5466 -2661
- Partials 0 132 +132
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `58.08% <67.14%> (+5.02%)` | :arrow_up: |
| javascript | `62.27% <49.55%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <ø> (+11.50%)` | :arrow_up: |
| [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `56.27% <ø> (+1.81%)` | :arrow_up: |
| [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `77.02% <0.00%> (+0.31%)` | :arrow_up: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <ø> (+4.76%)` | :arrow_up: |
| [.../src/common/components/Tooltip/Tooltip.stories.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvVG9vbHRpcC5zdG9yaWVzLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `98.66% <ø> (+6.66%)` | :arrow_up: |
| [superset-frontend/src/components/CachedLabel.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2FjaGVkTGFiZWwuanN4) | `42.10% <ø> (ø)` | |
| ... and [553 more](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=footer). Last update [bcaa484...121fc30](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579210486
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -29,14 +27,13 @@ import { DynamicPluginProvider } from 'src/components/DynamicPlugins';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import Menu from 'src/components/Menu/Menu';
-import FlashProvider from 'src/components/FlashProvider';
+import FlashProvider from 'src/components/FlashProvider/index';
Review comment:
I cloned the repository again and it worked
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580608860
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
>Whether you will export them later
In this case, the person who places an export simply adds the prefix. It’s not complex to do that, just read rsrs
or add more components in the same file. Always adding the prefix helps you not to have to worry about this.
> add more components in the same file
This is an extremely bad practice. Glad that not adding a prefix gets in the way to add more than one component per file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580507987
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@suddjian it's probably not a big issue anymore because of sourcemaps, but it used to be useful because function names are captured in error stacks which helps you to locate bad code in case of JS errors. I don't see additional benefits of arrow functions in creating React components so I always follow the named function style.
It's personal preference, so feel free to choose whatever style you like until we adopt a styleguide just for consistency.
@yardz Prefix would be preferred even if the props are not exported, because you never know whether you will export them later or add more components in the same file. Always adding the prefix helps you not to have to worry about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13220:
URL: https://github.com/apache/superset/pull/13220#issuecomment-782544966
# [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=h1) Report
> Merging [#13220](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=desc) (c8eb460) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `9.28%`.
> The diff coverage is `49.64%`.
[](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13220 +/- ##
==========================================
+ Coverage 53.06% 62.34% +9.28%
==========================================
Files 489 570 +81
Lines 17314 20672 +3358
Branches 4482 5425 +943
==========================================
+ Hits 9187 12888 +3701
+ Misses 8127 7573 -554
- Partials 0 211 +211
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `62.34% <49.64%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <ø> (+11.50%)` | :arrow_up: |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `39.18% <0.00%> (-37.53%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <ø> (+4.76%)` | :arrow_up: |
| [.../src/common/components/Tooltip/Tooltip.stories.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvVG9vbHRpcC5zdG9yaWVzLnRzeA==) | `0.00% <0.00%> (ø)` | |
| [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `96.00% <ø> (+4.00%)` | :arrow_up: |
| [superset-frontend/src/components/CachedLabel.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2FjaGVkTGFiZWwuanN4) | `42.10% <ø> (ø)` | |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `51.42% <0.00%> (-5.72%)` | :arrow_down: |
| [...t-frontend/src/components/DynamicPlugins/index.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRHluYW1pY1BsdWdpbnMvaW5kZXgudHN4) | `19.14% <ø> (-4.11%)` | :arrow_down: |
| ... and [611 more](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=footer). Last update [bcaa484...c8eb460](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578781870
##########
File path: superset-frontend/src/utils/useComponentDidMount/index.ts
##########
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export * from './useComponentDidMount';
Review comment:
Hooks should live in `src/common/hooks/`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579482405
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I think removing the unrelated changes would be best. I'm okay with having a `useComponentDidMount` hook around, as long as it is only used with code that is specifically dependent on that life cycle event, and isn't just using it to avoid lint warnings. A good example of this is the `LOG_ACTIONS_MOUNT_EXPLORER` event, which is concerned specifically the explorer container being mounted. I expect legit cases of `useComponentDidMount` to be very rare.
A side note on the tech debt in `ExploreViewContainer`:
A lot of the warnings here are left over from when I converted it from a class to a function component. I didn't have time to buff out all the edges there. I'm confident that better organization of state and business logic in this component would simplify the code and get rid of the warnings. For example, the `addHistory` function is called from multiple places, because in the class component we had to handle changes in that imperative way. But I suspect this functionality could instead be placed in a `useEffect` hook that sets the url based on state changes, which would be more declarative and robust.
That is what I was getting at when I said hooks are a new state management pattern that isn't based on component lifecycles. KCD discusses this in the post that @ktmud linked to as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579483862
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
I haven't had any problems with this style of component declaration before, the debugger seems to pick up the names just fine. Are you sure it's an issue?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578785178
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -29,14 +27,13 @@ import { DynamicPluginProvider } from 'src/components/DynamicPlugins';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import Menu from 'src/components/Menu/Menu';
-import FlashProvider from 'src/components/FlashProvider';
+import FlashProvider from 'src/components/FlashProvider/index';
Review comment:
That's strange we have quite a few modules using this pattern already. You probably needed to restart your webpack server as it's probably pointing to an outdated module resolution cache for this file (happens when you don't change the path but do change the file structure).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578781870
##########
File path: superset-frontend/src/utils/useComponentDidMount/index.ts
##########
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export * from './useComponentDidMount';
Review comment:
Hooks intended for common use should live in `src/common/hooks/`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578785178
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -29,14 +27,13 @@ import { DynamicPluginProvider } from 'src/components/DynamicPlugins';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import Menu from 'src/components/Menu/Menu';
-import FlashProvider from 'src/components/FlashProvider';
+import FlashProvider from 'src/components/FlashProvider/index';
Review comment:
That's strange we have quite a few modules using this pattern already. You probably needed to restart your webpack server as it's probably pointing to an outdated module resolution cache for this file (happens when you don't change the path but do change the file it previously resolved).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580395779
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -35,6 +35,7 @@ import {
setInLocalStorage,
} from 'src/utils/localStorageHelpers';
import { URL_PARAMS } from 'src/constants';
+import cls from 'classnames';
Review comment:
The codebase has been using `cx` as the import for this module.
##########
File path: superset-frontend/src/views/Store.tsx
##########
@@ -0,0 +1,35 @@
+/**
Review comment:
The pattern so far has been to only capitalize file names if they include a react component. I'm also not seeing any `jsx` in this file, so `store.ts` would be a better name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579519722
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@ktmud
> 1. Use named function instead of arrow functions, make it easier to find in debugging code
I prefer arrow function because it is more performative and easier to read. But in this case, it's just my taste. I can change this without any problems
> 2. Prefix props with component name, make it less likely to have export conflicts
I put a prefix when the interface is exported. This is not the case for this component/file. If someone in the future puts an export on the interface, they need to change the name too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: Tests for flash provider
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578667039
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -29,14 +27,13 @@ import { DynamicPluginProvider } from 'src/components/DynamicPlugins';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import Menu from 'src/components/Menu/Menu';
-import FlashProvider from 'src/components/FlashProvider';
+import FlashProvider from 'src/components/FlashProvider/index';
Review comment:
I this change necessary?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on pull request #13220:
URL: https://github.com/apache/superset/pull/13220#issuecomment-783302459
@nytai Is there anything more to approve this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579482405
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I think removing the unrelated changes would be best. I'm okay with having a `useComponentDidMount` hook around, as long as it is only used with code that is specifically dependent on that life cycle event, and isn't just using it to avoid lint warnings. A good example of this is the `LOG_ACTIONS_MOUNT_EXPLORER` event, which is concerned specifically with the component being mounted. I expect legit cases of `useComponentDidMount` to be very rare.
A side note on the tech debt in `ExploreViewContainer`:
A lot of the warnings here are left over from when I converted it from a class to a function component. I didn't have time to buff out all the edges there. I'm confident that better organization of state and business logic in this component would simplify the code and get rid of the warnings. For example, the `addHistory` function is called from multiple places, because in the class component we had to handle changes in that imperative way. But I suspect this functionality could instead be placed in a `useEffect` hook that sets the url based on state changes, which would be more declarative and robust.
That is what I was getting at when I said hooks are a new state management pattern that isn't based on component lifecycles. KCD discusses this in the post that @ktmud linked to as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578880651
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I think the point is that there is tech debt in these areas, and just applying `// eslint-disable-next-line react-hooks/exhaustive-deps` everywhere there is currently a warning is just further obfuscating that tech debt.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579488817
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -17,16 +17,15 @@
* under the License.
*/
import React from 'react';
-import withToasts from 'src/messageToasts/enhancers/withToasts';
+import { useToasts } from 'src/messageToasts/enhancers/withToasts';
+import { useComponentDidMount } from 'src/common/hooks/useComponentDidMount';
-type Message = Array<string>;
+type FlashMessageType = 'info' | 'alert' | 'danger' | 'warning' | 'success';
+export type FlashMessage = [FlashMessageType, string];
-interface CommonObject {
- flash_messages: Array<Message>;
-}
interface Props {
- children: Node;
- common: CommonObject;
+ children: JSX.Element;
+ messages: FlashMessage[];
Review comment:
Good idea to take `messages` instead of `common`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580608860
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
>Whether you will export them later
In this case, the person who places an export simply adds the prefix. It’s not complex to do that, just read rsrs
> add more components in the same file
This is an extremely bad practice. Glad that not adding a prefix gets in the way to add more than one component per file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578897372
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
Right, and having the warnings thrown is a constant reminder to revisit some of that tech debt. Not every PR can be perfect the first time around. In the case of this change, https://github.com/apache/superset/pull/13220/files#diff-9c9be0048f0d0d4f1bd547faad9f8732fe5a12df6fc185190681914251131cc9R67 I think the more correct behavior would be to add `infoEnable` and `resource` to the dependency array. It seems by applying your `useComponentDidMount` to that code in this PR, you are asserting that that the current behavior is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578880651
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I think the point is that there is tech debt in these areas, and just applying `// eslint-disable-next-line react-hooks/exhaustive-deps` everywhere there is currently a warning is just further obfuscating that tech debt. Additionally, wrapping the eslint disable in a custom hook seems like just another layer of obfuscation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai merged pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai merged pull request #13220:
URL: https://github.com/apache/superset/pull/13220
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578881574
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
I think this is the style we prefer:
```suggestion
export default function FlashProvide({ children, messages }: FlashProvideProps) {
```
1. Use named function instead of arrow functions, make it easier to find in debugging code
2. Prefix props with component name, make it less likely to have export conflicts
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@yardz Please refer to this post for why ignoring the exhaustive-deps warning is a bad idea: https://kentcdodds.com/blog/react-hooks-pitfalls#pitfall-2-not-using-or-ignoring-the-eslint-plugin
In my experience, most places where `useEffect` with an empty dependency list are used, it's either very easy to add the exhaustive deps without adding unnecessary re-renderings, or more correct to do so:
- `DynamicPlugins` : ESLint is not reporting error
- `useWindowSize`: it's more accurate to add `delayMs` because if you pass a different `delayMs`, it should add a new event handler (and remove the previous one).
- `ExploreViewContainer`: dependencies should be added as recommended, handler functions should be wrapped with `useCallback`. Because when the component get re-rendered, the old handlers would refer to closured variables in previously rendering cycle.
- `useListViewResource`: dependencies should be added because if the component that calls the hook re-renders and requests a different resource or passes in a different handler, there will be no API requests made. This makes the hook functionally incorrect. The correct way to avoid excessive requests is to cache API requests in another layer (either another hook or a new API client with cache support).
- `Welcome.tsx`: the only real dependency is `user_id`, adding it will not cause excessive re-rendering, but will also make it possible to re-render the page for different users (without unmounting the component).
Fixing the lint error is also very easy in most cases, you can simply hit "Cmd + ." in VSCode and use the recommended fix:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579482405
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I think removing the unrelated changes would be best. I'm okay with having a `useComponentDidMount` hook around, as long as it is only used with code that is specifically dependent on that life cycle event, and isn't just using it to avoid lint warnings. A good example of this is the LOG_ACTIONS_MOUNT_EXPLORER event, which is concerned specifically the explorer container being mounted. I expect legit cases of `useComponentDidMount` to be very rare.
A side note on the tech debt in `ExploreViewContainer`:
A lot of the warnings here are left over from when I converted it from a class to a function component. I didn't have time to buff out all the edges there. I'm confident that better organization of state and business logic in this component would simplify the code and get rid of the warnings. For example, the `addHistory` function is called from multiple places, because in the class component we had to handle changes in that imperative way. But I suspect this functionality could instead be placed in a `useEffect` hook that sets the url based on state changes, which would be more declarative and robust.
That is what I was getting at when I said hooks are a new state management pattern that isn't based on component lifecycles. KCD discusses this in the post that @ktmud linked to as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578892154
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@nytai If you need behavior similar to a "ComponentDidMount" it is a ``useEffect( effect, [] )``. But the ``[]`` will **always** generate an error with the rule and forces you to use a ``// eslint-disable-next-line react-hooks/exhaustive-deps``.
In **this case** it is not a technical debt, it is a design decision... I am extremely against "hiding" lint errors (and I hate use "eslint-disable"). But in this case, this is exactly the desired behavior, there is no escaping for it (I already tried, and a lot).
See the "FlashProvider", this is the desired behavior (apply the effect **exclusively** once).
The cases where I subsisted useEffect were the cases that it was being applied in this way. The hook is not wrong, it is not a technical debt and it is not an "anti pattern". If I applied it to a file that already contained the same logic and it was wrong it is not a problem with the hook or with this PR, it is a problem with the PR that accepted the file with the "bad implementation"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579519722
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@ktmud
> 1. Use named function instead of arrow functions, make it easier to find in debugging code
I prefer arrow function because it is more performative and easier to read. But in this case, it's just my taste. I can change this without any problems
> 2. Prefix props with component name, make it less likely to have export conflicts
I put a prefix when the interface is exported. This is not the case for this component/file. If someone in the future puts an export on the interface, they need to change the name too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580507987
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@suddjian it's probably not a big issue anymore because of sourcemaps, but it used to be useful because function names are captured in error stacks which helps you to locate bad code in case of JS errors. I don't see additional benefits of arrow functions in creating React components so I always follow the named function style.
It's personal preference, but feel free to choose whatever style you like until we adopt a styleguide just for consistency.
@yardz Prefix would be preferred even if the props are not exported, because you never know whether you will export them later or add more components in the same file. Always adding the prefix helps you not to have to worry about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578815464
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
I am wary of `useComponentDidMount` making it easier to ignore dependencies. Hooks are a new style of business logic organization that focuses on data flow rather than component lifecycles. Emulating the old component lifecycle focused design and silencing lint warnings is a step backwards in my view.
In this particular case, for example, rather than switching to `useComponentDidMount`, it would probably be more correct here to add `delayMs` to the dependencies array.
I do think there could be a place for `useComponentDidMount`, but only when the logic in question specifically depends on only running at component mount.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13220:
URL: https://github.com/apache/superset/pull/13220#issuecomment-782544966
# [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=h1) Report
> Merging [#13220](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=desc) (121fc30) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `1.72%`.
> The diff coverage is `64.83%`.
[](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13220 +/- ##
==========================================
+ Coverage 53.06% 54.78% +1.72%
==========================================
Files 489 466 -23
Lines 17314 15705 -1609
Branches 4482 4026 -456
==========================================
- Hits 9187 8604 -583
+ Misses 8127 7101 -1026
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `54.78% <64.83%> (+1.72%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <0.00%> (-0.09%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `75.67% <0.00%> (-1.04%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `50.00% <ø> (ø)` | |
| [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `92.00% <ø> (ø)` | |
| [superset-frontend/src/components/CachedLabel.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2FjaGVkTGFiZWwuanN4) | `42.10% <ø> (ø)` | |
| [...ontend/src/components/CertifiedIconWithTooltip.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2VydGlmaWVkSWNvbldpdGhUb29sdGlwLnRzeA==) | `0.00% <ø> (ø)` | |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `57.14% <0.00%> (ø)` | |
| ... and [209 more](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=footer). Last update [bcaa484...121fc30](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13220:
URL: https://github.com/apache/superset/pull/13220#issuecomment-782544966
# [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=h1) Report
> Merging [#13220](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=desc) (121fc30) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **decrease** coverage by `4.33%`.
> The diff coverage is `58.05%`.
[](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13220 +/- ##
==========================================
- Coverage 53.06% 48.72% -4.34%
==========================================
Files 489 466 -23
Lines 17314 15705 -1609
Branches 4482 4026 -456
==========================================
- Hits 9187 7652 -1535
+ Misses 8127 8053 -74
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `48.72% <58.05%> (-4.34%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <0.00%> (-0.09%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `52.77% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
| [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `72.97% <0.00%> (-3.74%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `50.00% <ø> (ø)` | |
| [...t-frontend/src/common/components/Tooltip/index.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL1Rvb2x0aXAvaW5kZXgudHN4) | `100.00% <ø> (ø)` | |
| [...perset-frontend/src/components/AlteredSliceTag.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQWx0ZXJlZFNsaWNlVGFnLmpzeA==) | `86.66% <ø> (-5.34%)` | :arrow_down: |
| [superset-frontend/src/components/CachedLabel.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2FjaGVkTGFiZWwuanN4) | `42.10% <ø> (ø)` | |
| [...ontend/src/components/CertifiedIconWithTooltip.tsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ2VydGlmaWVkSWNvbldpdGhUb29sdGlwLnRzeA==) | `0.00% <ø> (ø)` | |
| [...perset-frontend/src/components/CopyToClipboard.jsx](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29weVRvQ2xpcGJvYXJkLmpzeA==) | `45.71% <0.00%> (-11.43%)` | :arrow_down: |
| ... and [260 more](https://codecov.io/gh/apache/superset/pull/13220/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=footer). Last update [bcaa484...121fc30](https://codecov.io/gh/apache/superset/pull/13220?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578842648
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
+1 on not adding hacks just for suppressing warnings. These lint rules exist for a reason.
We should fix the dependencies rather than suppressing them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579033510
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@yardz Please refer to this post for why ignoring the exhaustive-deps warning is a bad idea: https://kentcdodds.com/blog/react-hooks-pitfalls#pitfall-2-not-using-or-ignoring-the-eslint-plugin
In my experience, most places where `useEffect` with an empty dependency list are used, it's either very easy to add the exhaustive deps without adding unnecessary re-renderings, or more correct to do so:
- `DynamicPlugins` : ESLint is not reporting error
- `useWindowSize`: it's more accurate to add `delayMs` because if you pass a different `delayMs`, it should add a new event handler (and remove the previous one).
- `ExploreViewContainer`: dependencies should be added as recommended, handler functions should be wrapped with `useCallback`. Because when the component get re-rendered, the old handlers would refer to closured variables in previously rendering cycle.
- `useListViewResource`: dependencies should be added because if the component that calls the hook re-renders and requests a different resource or passes in a different handler, there will be no API requests made. This makes the hook functionally incorrect. The correct way to avoid excessive requests is to cache API requests in another layer (either another hook or a new API client with cache support).
- `Welcome.tsx`: the only real dependency is `user_id`, adding it will not cause excessive re-rendering, but will also make it possible to re-render the page for different users (without unmounting the component).
Fixing the lint error is also very easy in most cases, you can simply hit "Cmd + ." in VSCode and use the recommended fix:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578864774
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@ktmud and @suddjian about the warning.
This ``[]`` causes a warning! And the only way to eliminate it is by adding `` // eslint-disable-next-line react-hooks/exhaustive-deps `` unfortunately it cannot be avoided.
Even in cases where it is desired to be executed only once, it is necessary to put "eslint-disable"... Of course, lint rules exist for a reason, but again, in this case the rule is not smart enough to understand that the desired behavior is exactly that.
So there are two choices, 1- put "eslint disable" in all the places where we want to execute the effect only once (currently we have something close to 7 files like this) or 2- centralize "eslint-disable" and reduce this as much as possible.
It seems to me that not adopting such a hook reinforces the use of "eslint-disable"... Which for me is a much worse practice.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579203541
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
Let's do this way. I will revert this commit where I put this hook in these other components. I didn't really read them, I just saw that they was using it that way and switched to hook... I just made this change to avoid repeated code. As it has already led to all this discussion, it has lost its purpose.
The hook is ok, the component is ok. It does not make sense to keep blocking the merge due to other components that are not the scope of this PR. Do you agree with that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580542997
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@nytai Indeed, JSX will add `name` to components created by arrow functions. My comments were based on past experience pre-React world.
Re: the automatically added `children` prob. I'm not sure how useful is this. A component either needs or not need the component prob. If it does not handle `children`, then `children` should not be an accepted prop; if it does, then it makes sense to be more explicit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] suddjian commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579482405
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
~I think removing the unrelated changes would be best.~ Thanks for pruning the PR scope! I'm okay with having a `useComponentDidMount` hook around, as long as it is only used with code that is specifically dependent on that life cycle event, and isn't just using it to avoid lint warnings. A good example of this is the `LOG_ACTIONS_MOUNT_EXPLORER` event, which is concerned specifically with the component being mounted. I expect legit cases of `useComponentDidMount` to be very rare.
A side note on the tech debt in `ExploreViewContainer`:
A lot of the warnings here are left over from when I converted it from a class to a function component. I didn't have time to buff out all the edges there. I'm confident that better organization of state and business logic in this component would simplify the code and get rid of the warnings. For example, the `addHistory` function is called from multiple places, because in the class component we had to handle changes in that imperative way. But I suspect this functionality could instead be placed in a `useEffect` hook that sets the url based on state changes, which would be more declarative and robust.
That is what I was getting at when I said hooks are a new state management pattern that isn't based on component lifecycles. KCD discusses this in the post that @ktmud linked to as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580511093
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@ktmud this is only true for anonymous default exports. In the case of `const CoolComponent: React.FC<Props> = props => ...` react is able to add the component name to stack traces based on the variable name, this also allows for the use of the `React.FC` interface which adds the `children` prop.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580507987
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@suddjian it's probably not a big issue anymore because of sourcemaps, but it used to be useful because function names are captured in error stacks which helps you locate bad code in case of JS errors. I don't see additional benefits of arrow functions in creating React components so I always follow the named function style.
It's personal preference, so feel free to choose whatever style you like until we adopt a styleguide just for consistency.
@yardz Prefix would be preferred even if the props are not exported, because you never know whether you will export them later or add more components in the same file. Always adding the prefix helps you not to have to worry about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579408355
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
It's a right call to make PRs atomic and not changing unrelated things unless necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580430920
##########
File path: superset-frontend/src/views/Store.tsx
##########
@@ -0,0 +1,35 @@
+/**
Review comment:
@nytai If you can do the merge I appreciate it. I can't do it :-D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578842648
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
+1 on not adding hacks just for suppressing warnings. These lint rules exist for a reason.
We should fix the dependency warnings rather than suppressing them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579413867
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@yardz Please refer to this post for why ignoring the exhaustive-deps warning is a bad idea: https://kentcdodds.com/blog/react-hooks-pitfalls#pitfall-2-not-using-or-ignoring-the-eslint-plugin
In my experience, most places where `useEffect` with an empty dependency list are used, it's either very easy to add the exhaustive deps without adding unnecessary re-renderings, or more correct to do so:
- `DynamicPlugins` : ESLint is not reporting error
- `useWindowSize`: it's more accurate to add `delayMs` because if you pass a different `delayMs`, it should add a new event handler (and remove the previous one).
- `ExploreViewContainer`: dependencies should be added as recommended, handler functions should be wrapped with `useCallback`. Because when the component get re-rendered, the old handlers would refer to closured variables in previously rendering cycle.
- `useListViewResource`: dependencies should be added because if the component that calls the hook re-renders and requests a different resource or passes in a different handler, there will be no API requests made. This makes the hook functionally incorrect. The correct way to avoid excessive requests is to cache API requests in another layer (either another hook or a new API client with cache support).
- `Welcome.tsx`: the only real dependency is `user_id`, adding it will not cause excessive re-rendering, but will also make it possible to re-render the page for different users (without unmounting the component).
Fixing the lint error is also very easy in most cases, you can simply hit "Cmd + ." in VSCode and use the recommended fix:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578874263
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
There are extremely complex class components in the application that use ComponentDidMount. Refactoring this and removing this behavior in one step will be extremely difficult and at a very high risk.
So it is absolutely not true that hooks are "a new style of business logic organization that focuses on data flow rather than component lifecycles". The main motivations are **"It's hard to reuse stateful logic between components"**, **"Complex components become hard to understand"** and **"Classes confuse both people and machines"** and they say the hooks are for: **"Hooks let you use more of React's features without classes"**.
Clearly their intention is to provide the same lifecicle/features of classes for functions, facilitate the reuse of code and reduce the size/complexity of the components.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580542997
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@nytai Indeed, JSX will add `name` to components created by arrow functions. My comments were based on past experience pre-React world.
Re: the automatically added `children` prob. I'm not sure how useful is this. A component either needs or does not need the `children` prop. If it does not handle `children`, then `children` should not be an accepted prop; if it does, then it makes sense to be more explicit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] nytai commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578897372
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
Right, and having the warnings thrown is a constant reminder to revisit some of that tech debt. Not every PR can be perfect the first time around. In the case of this change, https://github.com/apache/superset/pull/13220/files#diff-9c9be0048f0d0d4f1bd547faad9f8732fe5a12df6fc185190681914251131cc9R67 I think the more correct behavior would be to add `infoEnable` and `resource` to the dependency array. It seems by applying your `useComponentDidMount` to that code in this PR, you are asserting that that is the current behavior is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: Tests for flash provider
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578671174
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -29,14 +27,13 @@ import { DynamicPluginProvider } from 'src/components/DynamicPlugins';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import Menu from 'src/components/Menu/Menu';
-import FlashProvider from 'src/components/FlashProvider';
+import FlashProvider from 'src/components/FlashProvider/index';
Review comment:
When I removed the "/index" it started to point out an error in the typescript. I believe it is some configuration in the webpack.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r580409734
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -35,6 +35,7 @@ import {
setInLocalStorage,
} from 'src/utils/localStorageHelpers';
import { URL_PARAMS } from 'src/constants';
+import cls from 'classnames';
Review comment:
Done
##########
File path: superset-frontend/src/views/Store.tsx
##########
@@ -0,0 +1,35 @@
+/**
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578857107
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
@suddjian I understand your point, however I did not refactor this file (if I had even done it I would have passed it to typescript).
I just kept the same logic / behavior that already existed, if "this behavior is bad" I believe it would be a matter for another PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579210748
##########
File path: superset-frontend/src/utils/useComponentDidMount/index.ts
##########
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export * from './useComponentDidMount';
Review comment:
moved
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: tests for FlashProvider component
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r578671174
##########
File path: superset-frontend/src/views/App.tsx
##########
@@ -29,14 +27,13 @@ import { DynamicPluginProvider } from 'src/components/DynamicPlugins';
import ErrorBoundary from 'src/components/ErrorBoundary';
import Loading from 'src/components/Loading';
import Menu from 'src/components/Menu/Menu';
-import FlashProvider from 'src/components/FlashProvider';
+import FlashProvider from 'src/components/FlashProvider/index';
Review comment:
@nytai When I removed the "/index" it started to point out an error in the typescript. I believe it is some configuration in the webpack.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yardz commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
yardz commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579519722
##########
File path: superset-frontend/src/components/FlashProvider/index.tsx
##########
@@ -37,21 +36,19 @@ const flashObj = {
success: 'addSuccessToast',
};
-class FlashProvider extends React.PureComponent<Props> {
- componentDidMount() {
- const flashMessages = this.props.common.flash_messages;
- flashMessages.forEach(message => {
+const FlashProvider: React.FC<Props> = ({ children, messages }) => {
Review comment:
@ktmud
> 1. Use named function instead of arrow functions, make it easier to find in debugging code
I prefer arrow function because it is more performatic and easier to read. But in this case, it's just my taste. I can change this without any problems
> 2. Prefix props with component name, make it less likely to have export conflicts
I put a prefix when the interface is exported. This is not the case for this component/file. If someone in the future puts an export on the interface, they need to change the name too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a change in pull request #13220: test: Switching FlashProvider to function component & creating tests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13220:
URL: https://github.com/apache/superset/pull/13220#discussion_r579408355
##########
File path: superset-frontend/src/explore/components/ExploreViewContainer.jsx
##########
@@ -146,11 +147,11 @@ const getWindowSize = () => ({
function useWindowSize({ delayMs = 250 } = {}) {
const [size, setSize] = useState(getWindowSize());
- useEffect(() => {
+ useComponentDidMount(() => {
const onWindowResize = debounce(() => setSize(getWindowSize()), delayMs);
window.addEventListener('resize', onWindowResize);
return () => window.removeEventListener('resize', onWindowResize);
- }, []);
+ });
Review comment:
It's a right call to make each PR independent and not changing unrelated things unless necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org