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/03/17 20:55:20 UTC

[GitHub] [superset] ktmud opened a new pull request #13679: fix: use FeatureFlags in @superset-ui/core

ktmud opened a new pull request #13679:
URL: https://github.com/apache/superset/pull/13679


   ### SUMMARY
   
   Feature flag typing [has been added to `@superset-ui/core`](https://github.com/apache-superset/superset-ui/pull/982), since there is now two copies of the same typing, sometimes it leads to TS errors when `npm link` is used or when in Jest.
   
   This change means all future feature flags used in the front-end have to be added to `@superset-ui` and go through the npm publish process. This is inconvenient but probably necessary to ensure correctness and consistency. Considering we are merging `superset-ui` to this repo soon, this should not be that much of a problem.
   
   cc @graceguo-supercat @villebro 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   N/A
   
   ### TEST PLAN
   
   1. `npm link ../superset-ui/packages/superset-ui-core`
   2. `npm run type`
   3. You will see this error in `master`: 
   
   ```
   node_modules/@superset-ui/core/src/utils/featureFlags.ts:53:5 - error TS2717: Subsequent property declarations must have the same type.  Property 'featureFlags' must be of type 'FeatureFlagMap', but here has type 'FeatureFlagMap'.
   
   53     featureFlags: FeatureFlagMap;
          ~~~~~~~~~~~~
   
     src/featureFlags.ts:54:5
       54     featureFlags: FeatureFlagMap;
              ~~~~~~~~~~~~
       'featureFlags' was also declared here.
   
   
   Found 1 error.
   ```
   
   This PR should fix it.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: https://github.com/apache-superset/superset-ui/pull/982
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
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] etr2460 commented on a change in pull request #13679: fix: use FeatureFlags in @superset-ui/core

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #13679:
URL: https://github.com/apache/superset/pull/13679#discussion_r596548086



##########
File path: superset-frontend/src/setup/setupApp.ts
##########
@@ -22,8 +22,16 @@ import { SupersetClient } from '@superset-ui/core';
 import {
   getClientErrorObject,
   ClientErrorObject,
-} from '../utils/getClientErrorObject';
-import setupErrorMessages from './setupErrorMessages';
+} from 'src/utils/getClientErrorObject';
+import setupErrorMessages from 'src/setup/setupErrorMessages';
+
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+declare global {
+  interface Window {
+    $: any;
+    jQuery: any;
+  }
+}

Review comment:
       is this related to the feature flag change? i didn't see how




----------------------------------------------------------------
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 #13679: fix: use FeatureFlags in @superset-ui/core

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #13679:
URL: https://github.com/apache/superset/pull/13679#discussion_r597026978



##########
File path: superset-frontend/src/setup/setupApp.ts
##########
@@ -22,8 +22,16 @@ import { SupersetClient } from '@superset-ui/core';
 import {
   getClientErrorObject,
   ClientErrorObject,
-} from '../utils/getClientErrorObject';
-import setupErrorMessages from './setupErrorMessages';
+} from 'src/utils/getClientErrorObject';
+import setupErrorMessages from 'src/setup/setupErrorMessages';
+
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+declare global {
+  interface Window {
+    $: any;
+    jQuery: any;
+  }
+}

Review comment:
       It's not, but these globals used to be declared in the `featureFlags.ts` file. I moved them closer to where they are actually used.




-- 
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[bot] commented on pull request #13679: fix: use FeatureFlags in @superset-ui/core

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #13679:
URL: https://github.com/apache/superset/pull/13679#issuecomment-801510125


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13679?src=pr&el=h1) Report
   > Merging [#13679](https://codecov.io/gh/apache/superset/pull/13679?src=pr&el=desc) (3391eba) into [master](https://codecov.io/gh/apache/superset/commit/abf2c87115bc34df1db102ae3d64bc011dc93ff6?el=desc) (abf2c87) will **decrease** coverage by `4.01%`.
   > The diff coverage is `50.00%`.
   
   > :exclamation: Current head 3391eba differs from pull request most recent head 0bf5706. Consider uploading reports for the commit 0bf5706 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13679/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13679?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13679      +/-   ##
   ==========================================
   - Coverage   77.10%   73.08%   -4.02%     
   ==========================================
     Files         921      611     -310     
     Lines       46745    21695   -25050     
     Branches     5733     5732       -1     
   ==========================================
   - Hits        36043    15856   -20187     
   + Misses      10567     5704    -4863     
     Partials      135      135              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `56.54% <ø> (-0.07%)` | :arrow_down: |
   | javascript | `63.11% <50.00%> (-0.04%)` | :arrow_down: |
   | mysql | `?` | |
   | python | `?` | |
   
   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/13679?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/setup/setupApp.ts](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQXBwLnRz) | `25.00% <0.00%> (ø)` | |
   | [superset-frontend/src/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVGbGFncy50cw==) | `100.00% <100.00%> (ø)` | |
   | [superset-frontend/src/filters/utils.ts](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvdXRpbHMudHM=) | `88.88% <0.00%> (-11.12%)` | :arrow_down: |
   | [superset/views/dashboard/views.py](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGFzaGJvYXJkL3ZpZXdzLnB5) | | |
   | [superset/viz.py](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | | |
   | [superset/db\_engine\_specs/oracle.py](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL29yYWNsZS5weQ==) | | |
   | [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | | |
   | [superset/utils/urls.py](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvdXJscy5weQ==) | | |
   | [superset/reports/notifications/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL19faW5pdF9fLnB5) | | |
   | ... and [304 more](https://codecov.io/gh/apache/superset/pull/13679/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13679?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/13679?src=pr&el=footer). Last update [abf2c87...0bf5706](https://codecov.io/gh/apache/superset/pull/13679?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 merged pull request #13679: fix: use FeatureFlags in @superset-ui/core

Posted by GitBox <gi...@apache.org>.
ktmud merged pull request #13679:
URL: https://github.com/apache/superset/pull/13679


   


-- 
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