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 2020/09/11 05:27:32 UTC
[GitHub] [incubator-superset] ktmud opened a new pull request #10837: feat: move ace-editor and mathjs to async modules
ktmud opened a new pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837
### SUMMARY
Follow up on #10831, move `brace`/`react-ace` and `mathjs` to async modules so that the initial page load for dashboards are faster.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
`brace.js` loads only after users started to edit a Markdown section:
Optimized bundle config further reduces the overall bundle size:
Initial page load time in Fast 3G network reduced by 30% (from 9.75s to 6.91s)
### TEST PLAN
- Added Cypress tests
Expected behaviors:
1. Dashboards should not load `brace.js` and `mathjs` by default.
2. Entering edit mode in dashboards with Markdown widgets should preload `brace.js`, even when users haven't started editing the markdown.
3. Dashboards with Line charts should load `mathjs` automatically.
4. In explore view, `mathjs` will load only when users open the annotation layer popup.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue:
- [ ] 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] [incubator-superset] mistercrunch commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-691425103
----------------------------------------------------------------
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] [incubator-superset] ktmud merged pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud merged pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a3e2e65121fadc8ca507c490eb5a3b26d8153041?el=desc) will **decrease** coverage by `4.78%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
- Coverage 65.42% 60.63% -4.79%
==========================================
Files 804 380 -424
Lines 37942 24057 -13885
Branches 3561 0 -3561
==========================================
- Hits 24823 14588 -10235
+ Misses 13010 9469 -3541
+ Partials 109 0 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `60.63% <ø> (-0.60%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `86.66% <0.00%> (-1.67%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.46% <0.00%> (-0.29%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.22% <0.00%> (-0.25%)` | :arrow_down: |
| ... and [428 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [a3e2e65...79ce5a4](https://codecov.io/gh/apache/incubator-superset/pull/10837?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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7cd96edcdf66c4f5ee6c1d5eb75987e5ddda03cc?el=desc) will **decrease** coverage by `4.84%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
- Coverage 65.46% 60.62% -4.85%
==========================================
Files 809 380 -429
Lines 38154 24068 -14086
Branches 3605 0 -3605
==========================================
- Hits 24979 14591 -10388
+ Misses 13066 9477 -3589
+ Partials 109 0 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `60.62% <ø> (-0.72%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-10.77%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `86.66% <0.00%> (-1.67%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.48% <0.00%> (-0.28%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.22% <0.00%> (-0.25%)` | :arrow_down: |
| ... and [413 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [7cd96ed...5e0a128](https://codecov.io/gh/apache/incubator-superset/pull/10837?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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/045335a687580df0fc835824cb3381367d737cda?el=desc) will **increase** coverage by `2.02%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
+ Coverage 59.32% 61.34% +2.02%
==========================================
Files 776 380 -396
Lines 37060 24086 -12974
Branches 3310 0 -3310
==========================================
- Hits 21986 14776 -7210
+ Misses 14891 9310 -5581
+ Partials 183 0 -183
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #python | `61.34% <ø> (-0.02%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.61% <0.00%> (-0.68%)` | :arrow_down: |
| [...set-frontend/src/dashboard/util/injectCustomCss.js](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2luamVjdEN1c3RvbUNzcy5qcw==) | | |
| [...board/util/getSelectedChartIdForFilterScopeTree.js](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldFNlbGVjdGVkQ2hhcnRJZEZvckZpbHRlclNjb3BlVHJlZS5qcw==) | | |
| [.../src/SqlLab/components/EstimateQueryCostButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0VzdGltYXRlUXVlcnlDb3N0QnV0dG9uLmpzeA==) | | |
| [...uperset-frontend/src/components/TooltipWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVG9vbHRpcFdyYXBwZXIuanN4) | | |
| [...frontend/src/dashboard/actions/dashboardFilters.js](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZEZpbHRlcnMuanM=) | | |
| [...src/dashboard/components/DeleteComponentButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0RlbGV0ZUNvbXBvbmVudEJ1dHRvbi5qc3g=) | | |
| [...end/src/dashboard/util/getKeyForFilterScopeTree.js](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEtleUZvckZpbHRlclNjb3BlVHJlZS5qcw==) | | |
| [.../src/explore/components/FilterDefinitionOption.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9GaWx0ZXJEZWZpbml0aW9uT3B0aW9uLmpzeA==) | | |
| [...nd/src/dashboard/util/getComponentWidthFromDrop.js](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldENvbXBvbmVudFdpZHRoRnJvbURyb3AuanM=) | | |
| ... and [371 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [045335a...21616b3](https://codecov.io/gh/apache/incubator-superset/pull/10837?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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7cd96edcdf66c4f5ee6c1d5eb75987e5ddda03cc?el=desc) will **decrease** coverage by `5.17%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
- Coverage 65.46% 60.29% -5.18%
==========================================
Files 809 380 -429
Lines 38154 24057 -14097
Branches 3605 0 -3605
==========================================
- Hits 24979 14506 -10473
+ Misses 13066 9551 -3515
+ Partials 109 0 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `60.29% <ø> (-1.04%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `31.91% <0.00%> (-59.58%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `59.64% <0.00%> (-22.81%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-10.77%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/databases/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `81.38% <0.00%> (-7.98%)` | :arrow_down: |
| [superset/databases/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2Rhby5weQ==) | `94.11% <0.00%> (-5.89%)` | :arrow_down: |
| [superset/views/database/validators.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvdmFsaWRhdG9ycy5weQ==) | `78.94% <0.00%> (-5.27%)` | :arrow_down: |
| ... and [429 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [7cd96ed...5e0a128](https://codecov.io/gh/apache/incubator-superset/pull/10837?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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r487171528
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r487171528
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
----------------------------------------------------------------
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] [incubator-superset] kristw commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
kristw commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488903351
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,34 +219,34 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
'@hot-loader.*',
'webpack.*',
'@?babel.*',
'lodash.*',
'antd',
'@ant-design.*',
'.*bootstrap',
+ 'react-bootstrap-slider',
'moment',
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
Review comment:
typo
`||interpolateformat` => `|interpolate|format`
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
----------------------------------------------------------------
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] [incubator-superset] rusackas commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488297009
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
##########
@@ -33,6 +33,14 @@ describe('Visualization > Line', () => {
cy.get('.alert-warning').contains(`"Metrics" cannot be empty`);
});
+ it('should preload mathjs', () => {
+ cy.get('script[src*="mathjs"]').should('have.length', 1);
Review comment:
The tests you have to show whether or not mathjs has been pre-loaded are great. Do you think it's worth adding a test that switches the chart type in explore, and checks to make sure it _gets_ loaded on-demand? Just doesn't seem there's a test where the length goes from 0 to 1.
----------------------------------------------------------------
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] [incubator-superset] mistercrunch commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-691425103
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r487171528
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
----------------------------------------------------------------
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] [incubator-superset] mistercrunch commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-691425103
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488296834
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/control.test.ts
##########
@@ -37,7 +37,7 @@ describe('Datasource control', () => {
cy.get('#datasource_menu').click();
cy.get('a').contains('Edit Datasource').click();
// create new metric
- cy.get('button').contains('Add Item').click();
+ cy.get('table button').contains('Add Item', { timeout: 10000 }).click();
Review comment:
TBH I'm not a big fan of the `data-test` attributes since it exposes dev stuff to production---not that it matters much though. I personally prefer HTML tag selector + `contains(...)` because: 1) they are more or less more stable than CSS class names, 2) tests probably do need to break when copy text or underlying data changes.
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488906328
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,34 +219,34 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
'@hot-loader.*',
'webpack.*',
'@?babel.*',
'lodash.*',
'antd',
'@ant-design.*',
'.*bootstrap',
+ 'react-bootstrap-slider',
'moment',
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
Review comment:
Wow, just wow... Thanks for spotting 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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
----------------------------------------------------------------
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] [incubator-superset] rusackas commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488298983
##########
File path: superset-frontend/src/dashboard/components/CssEditor.jsx
##########
@@ -19,12 +19,9 @@
import React from 'react';
import PropTypes from 'prop-types';
import Select from 'src/components/Select';
-import AceEditor from 'react-ace';
-import 'brace/mode/css';
-import 'brace/theme/github';
import { t } from '@superset-ui/core';
-
-import ModalTrigger from '../../components/ModalTrigger';
+import ModalTrigger from 'src/components/ModalTrigger';
+import { CssEditor as AceCssEditor } from 'src/components/AsyncAceEditor';
Review comment:
Why not just import it as CssEditor? For this or other editors, the Ace branding seems relatively unimportant (and who knows, maybe something we'll pivot away from one day).
----------------------------------------------------------------
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] [incubator-superset] rusackas commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488300385
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/control.test.ts
##########
@@ -37,7 +37,7 @@ describe('Datasource control', () => {
cy.get('#datasource_menu').click();
cy.get('a').contains('Edit Datasource').click();
// create new metric
- cy.get('button').contains('Add Item').click();
+ cy.get('table button').contains('Add Item', { timeout: 10000 }).click();
Review comment:
Fair point about production noise... I was thinking we should follow up and clean them out using something like [this](https://www.npmjs.com/package/babel-plugin-jsx-remove-data-test-id). But I don't want to hijack your PR threads for this discussion ;) You just made it cross my mind!
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488304160
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
##########
@@ -33,6 +33,14 @@ describe('Visualization > Line', () => {
cy.get('.alert-warning').contains(`"Metrics" cannot be empty`);
});
+ it('should preload mathjs', () => {
+ cy.get('script[src*="mathjs"]').should('have.length', 1);
Review comment:
Good idea. I just add a test case for VizTypeControl and included the logic on script loading.
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
----------------------------------------------------------------
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] [incubator-superset] mistercrunch commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-691425103
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a3e2e65121fadc8ca507c490eb5a3b26d8153041?el=desc) will **decrease** coverage by `4.78%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
- Coverage 65.42% 60.63% -4.79%
==========================================
Files 804 380 -424
Lines 37942 24057 -13885
Branches 3561 0 -3561
==========================================
- Hits 24823 14588 -10235
+ Misses 13010 9469 -3541
+ Partials 109 0 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `60.63% <ø> (-0.60%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `86.66% <0.00%> (-1.67%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.46% <0.00%> (-0.29%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.22% <0.00%> (-0.25%)` | :arrow_down: |
| ... and [428 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [a3e2e65...79ce5a4](https://codecov.io/gh/apache/incubator-superset/pull/10837?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] [incubator-superset] rusackas commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488304623
##########
File path: superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
##########
@@ -27,10 +27,15 @@ import {
import { connect } from 'react-redux';
import { t, withTheme } from '@superset-ui/core';
import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
-import { getChartKey } from '../../exploreUtils';
-import { runAnnotationQuery } from '../../../chart/chartAction';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+import { getChartKey } from 'src/explore/exploreUtils';
+import { runAnnotationQuery } from 'src/chart/chartAction';
-import AnnotationLayer from './AnnotationLayer';
+const AnnotationLayer = AsyncEsmComponent(
+ () => import('./AnnotationLayer'),
+ // size of overlay inner content
+ () => <div style={{ width: 450, height: 368 }} />,
Review comment:
Could/should these be based on % values or use a flexbox layout (using the built-in "stretch" features) to avoid hard-coding these dimensions?
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488838825
##########
File path: superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
##########
@@ -27,10 +27,15 @@ import {
import { connect } from 'react-redux';
import { t, withTheme } from '@superset-ui/core';
import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';
-import { getChartKey } from '../../exploreUtils';
-import { runAnnotationQuery } from '../../../chart/chartAction';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+import { getChartKey } from 'src/explore/exploreUtils';
+import { runAnnotationQuery } from 'src/chart/chartAction';
-import AnnotationLayer from './AnnotationLayer';
+const AnnotationLayer = AsyncEsmComponent(
+ () => import('./AnnotationLayer'),
+ // size of overlay inner content
+ () => <div style={{ width: 450, height: 368 }} />,
Review comment:
The goal here is to make sure the popover renders at the right position so that the left caret always points to the popover trigger when the content of the popover is fully loaded. We wouldn't want the popover to "stretch" since there is no way to update its position after it was re-rendered.
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter edited a comment on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a3e2e65121fadc8ca507c490eb5a3b26d8153041?el=desc) will **decrease** coverage by `4.74%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
- Coverage 65.42% 60.67% -4.75%
==========================================
Files 804 380 -424
Lines 37942 24068 -13874
Branches 3561 0 -3561
==========================================
- Hits 24823 14604 -10219
+ Misses 13010 9464 -3546
+ Partials 109 0 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `60.67% <ø> (-0.56%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `87.50% <0.00%> (-0.84%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.61% <0.00%> (-0.14%)` | :arrow_down: |
| [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | | |
| [superset-frontend/src/components/Menu/Menu.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9NZW51LnRzeA==) | | |
| ... and [422 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [a3e2e65...79ce5a4](https://codecov.io/gh/apache/incubator-superset/pull/10837?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] [incubator-superset] rusackas commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488286150
##########
File path: superset-frontend/cypress-base/cypress/integration/explore/control.test.ts
##########
@@ -37,7 +37,7 @@ describe('Datasource control', () => {
cy.get('#datasource_menu').click();
cy.get('a').contains('Edit Datasource').click();
// create new metric
- cy.get('button').contains('Add Item').click();
+ cy.get('table button').contains('Add Item', { timeout: 10000 }).click();
Review comment:
FWIW, I'm planning to go through and replace lots of these cypress DOM selectors with `data-test` attribute selectors, so things are more explicit and easier to maintain test integrity/auditability. If you have feelings on that, let me know :)
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r488299320
##########
File path: superset-frontend/src/dashboard/components/CssEditor.jsx
##########
@@ -19,12 +19,9 @@
import React from 'react';
import PropTypes from 'prop-types';
import Select from 'src/components/Select';
-import AceEditor from 'react-ace';
-import 'brace/mode/css';
-import 'brace/theme/github';
import { t } from '@superset-ui/core';
-
-import ModalTrigger from '../../components/ModalTrigger';
+import ModalTrigger from 'src/components/ModalTrigger';
+import { CssEditor as AceCssEditor } from 'src/components/AsyncAceEditor';
Review comment:
Because this file itself is named `CssEditor`. Thought it'd be beneficial to differentiate.
----------------------------------------------------------------
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] [incubator-superset] ktmud commented on a change in pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#discussion_r487171528
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
##########
File path: superset-frontend/src/components/AsyncEsmComponent.tsx
##########
@@ -0,0 +1,123 @@
+/**
+ * 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.
+ */
+import React, { useEffect, useState, RefObject } from 'react';
+import Loading from './Loading';
+
+type PlaceholderProps = {
+ showLoadingForImport: boolean;
+ width?: string | number;
+ height?: string | number;
+} & {
+ [key: string]: any;
+};
+
+function DefaultPlaceholder({
+ width,
+ height,
+ showLoadingForImport,
+}: PlaceholderProps) {
+ return (
+ // since `width` defaults to 100%, we can display the placeholder once
+ // height is specified.
+ (height && (
+ <div style={{ width, height }}>
+ {showLoadingForImport && <Loading position="floating" />}
Review comment:
This is for displaying a loading spinner while the async module is still loading:
E.g.:
Normally users wouldn't see this because most places we used the `AsyncEsmComponent` have on-demand preloading.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -55,9 +55,12 @@ const output = {
if (isDevMode) {
output.filename = '[name].[hash:8].entry.js';
output.chunkFilename = '[name].[hash:8].chunk.js';
-} else {
+} else if (nameChunks) {
output.filename = '[name].[chunkhash].entry.js';
output.chunkFilename = '[name].[chunkhash].chunk.js';
+} else {
+ output.filename = '[name].[chunkhash].entry.js';
+ output.chunkFilename = '[chunkhash].chunk.js';
Review comment:
Don't add names to `chunkFiles` (they are just single digit numbers anyway).
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
Review comment:
Set default `tabSize` for all AceEditor. Previously there were some places where `tabSize` was not set and would default to `4`.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -232,16 +240,12 @@ const config = {
'jquery',
'core-js.*',
'@emotion.*',
- 'd3.*',
+ 'd3',
+ 'd3-(array|color|scale||interpolateformat|selection|collection|time|time-format)',
].join('|')})/`,
),
},
// bundle large libraries separately
- brace: {
- name: 'brace',
- test: /\/node_modules\/(brace|react-ace)\//,
- priority: 40,
- },
Review comment:
Different `brace` modes/themes now load separately and asynchronously.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
width="100%"
+ height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
- enableLiveAutocompletion
Review comment:
Removed this prop according to a console warning from Ace.
##########
File path: superset-frontend/src/explore/components/controls/TextAreaControl.jsx
##########
@@ -73,18 +73,18 @@ export default class TextAreaControl extends React.Component {
}
renderEditor(inModal = false) {
const value = this.props.value || '';
+ const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
return (
- <AceEditor
+ <TextAreaEditor
mode={this.props.language}
- theme="textmate"
style={{ border: '1px solid #CCC' }}
- minLines={inModal ? 40 : this.props.minLines}
+ minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
- onChange={this.onAceChange.bind(this)}
+ onChange={this.onAceChangeDebounce}
Review comment:
Added `debounce` to text inputs in Explore view's Datasource editor.
##########
File path: superset-frontend/webpack.config.js
##########
@@ -214,13 +219,16 @@ const config = {
'react',
'react-dom',
'prop-types',
+ 'react-prop-types',
+ 'prop-types-extra',
'redux',
'react-redux',
'react-hot-loader',
'react-select',
'react-sortable-hoc',
'react-virtualized',
'react-table',
+ 'react-ace',
Review comment:
`react-ace` itself is quite small.
##########
File path: superset-frontend/src/components/AsyncAceEditor.tsx
##########
@@ -0,0 +1,166 @@
+/**
+ * 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.
+ */
+import React from 'react';
+import {
+ Editor,
+ IEditSession,
+ Position,
+ TextMode as OrigTextMode,
+} from 'brace';
+import AceEditor, { AceEditorProps } from 'react-ace';
+import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
+
+type TextMode = OrigTextMode & { $id: string };
+
+/**
+ * Async loaders to import brace modules. Must manually create call `import(...)`
+ * promises because webpack can only analyze asycn imports statically.
+ */
+const aceModuleLoaders = {
+ 'mode/sql': () => import('brace/mode/sql'),
+ 'mode/markdown': () => import('brace/mode/markdown'),
+ 'mode/css': () => import('brace/mode/css'),
+ 'mode/json': () => import('brace/mode/json'),
+ 'mode/yaml': () => import('brace/mode/yaml'),
+ 'mode/html': () => import('brace/mode/html'),
+ 'mode/javascript': () => import('brace/mode/javascript'),
+ 'theme/textmate': () => import('brace/theme/textmate'),
+ 'theme/github': () => import('brace/theme/github'),
+ 'ext/language_tools': () => import('brace/ext/language_tools'),
+};
+export type AceModule = keyof typeof aceModuleLoaders;
+
+export interface AceCompleterKeywordData {
+ name: string;
+ value: string;
+ score: number;
+ meta: string;
+}
+export interface AceCompleterKeyword extends AceCompleterKeywordData {
+ completer?: {
+ insertMatch: (editor: Editor, data: AceCompleterKeywordData) => void;
+ };
+}
+
+export type AsyncAceEditorProps = AceEditorProps & {
+ keywords?: AceCompleterKeyword[];
+};
+
+export type AceEditorMode = 'sql';
+export type AceEditorTheme = 'textmate' | 'github';
+export type AsyncAceEditorOptions = {
+ defaultMode?: AceEditorMode;
+ defaultTheme?: AceEditorTheme;
+ defaultTabSize?: number;
+};
+
+/**
+ * Get an async AceEditor with automatical loading of specified ace modules.
+ */
+export default function AsyncAceEditor(
+ aceModules: AceModule[],
+ { defaultMode, defaultTheme, defaultTabSize = 2 }: AsyncAceEditorOptions = {},
+) {
+ return AsyncEsmComponent(async () => {
+ const { default: ace } = await import('brace');
+ const { default: ReactAceEditor } = await import('react-ace');
+
+ await Promise.all(aceModules.map(x => aceModuleLoaders[x]()));
+
+ const inferredMode =
+ defaultMode ||
+ aceModules.find(x => x.startsWith('mode/'))?.replace('mode/', '');
+ const inferredTheme =
+ defaultTheme ||
+ aceModules.find(x => x.startsWith('theme/'))?.replace('theme/', '');
+
+ return React.forwardRef<AceEditor, AsyncAceEditorProps>(
+ function ExtendedAceEditor(
+ {
+ keywords,
+ mode = inferredMode,
+ theme = inferredTheme,
+ tabSize = defaultTabSize,
+ ...props
+ },
+ ref,
+ ) {
+ if (keywords) {
+ const langTools = ace.acequire('ace/ext/language_tools');
+ const completer = {
+ getCompletions: (
+ editor: AceEditor,
+ session: IEditSession,
+ pos: Position,
+ prefix: string,
+ callback: (error: null, wordList: object[]) => void,
+ ) => {
+ if ((session.getMode() as TextMode).$id === `ace/mode/${mode}`) {
Review comment:
Enable completion for only the related language.
----------------------------------------------------------------
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] [incubator-superset] codecov-commenter commented on pull request #10837: feat: move ace-editor and mathjs to async modules
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10837:
URL: https://github.com/apache/incubator-superset/pull/10837#issuecomment-690967808
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=h1) Report
> Merging [#10837](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a3e2e65121fadc8ca507c490eb5a3b26d8153041?el=desc) will **decrease** coverage by `4.78%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10837?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10837 +/- ##
==========================================
- Coverage 65.42% 60.63% -4.79%
==========================================
Files 804 380 -424
Lines 37942 24057 -13885
Branches 3561 0 -3561
==========================================
- Hits 24823 14588 -10235
+ Misses 13010 9469 -3541
+ Partials 109 0 -109
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `?` | |
| #python | `60.63% <ø> (-0.60%)` | :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/incubator-superset/pull/10837?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.16% <0.00%> (-12.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `86.66% <0.00%> (-1.67%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.46% <0.00%> (-0.29%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.22% <0.00%> (-0.25%)` | :arrow_down: |
| ... and [428 more](https://codecov.io/gh/apache/incubator-superset/pull/10837/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10837?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/incubator-superset/pull/10837?src=pr&el=footer). Last update [a3e2e65...79ce5a4](https://codecov.io/gh/apache/incubator-superset/pull/10837?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