ktmud commented on a change in pull request #11199:
URL:
https://github.com/apache/incubator-superset/pull/11199#discussion_r521887850
##########
File path:
superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
##########
@@ -23,11 +23,11 @@ describe('AdhocFilters', () => {
cy.route('GET', '/superset/explore_json/**').as('getJson');
cy.route('POST', '/superset/explore_json/**').as('postJson');
cy.route('GET', '/superset/filter/table/*/name').as('filterValues');
- });
-
- it('Should not load mathjs when not needed', () => {
cy.visitChartByName('Boys'); // a table chart
cy.verifySliceSuccess({ waitAlias: '@postJson' });
+ });
+
+ xit('Should not load mathjs when not needed', () => {
Review comment:
The test is failing because of this import chain:
`@superset-ui/plugin-chart-echarts -> Timeseries -> transformProps ->
utils/annotation -> mathjs`. Unlike `loadChart`, `transformProps` is not loaded
asynchronously, so Webpack could not defer the loading of `mathjs`, which means
it will be loaded for all first-visits on almost all pages, undoing part of the
optimization we did in #10837 .
The test is there to make sure this optimization still works. It didn't
break because it was flaky. It caught the breaking changes it is supposed to
catch.
It's OK to merge this PR as is for now, but I'd recommend moving the
`mathjs` logic to the chart renderer so that we could continue benefiting from
code split and asynchronous loading.
In general I think `transformProps` should be as lightweight as possible and
preferably only return scalars. Most complex logics should be dealt with either
by the chart renderer itself or at another data-processing layer on top of the
chart. This way it's easier to reuse your chart at other places outside of
Superset charting plugins.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
