[GitHub] [superset] villebro commented on a change in pull request #16701: chore: Upgrade Webpack to v5

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #16701:
URL: https://github.com/apache/superset/pull/16701#discussion_r710807844



##########
File path: superset-frontend/cypress-base/cypress/integration/dashboard/markdown.test.ts
##########
@@ -25,10 +25,6 @@ describe('Dashboard edit markdown', () => {
   });
 
   it('should load AceEditor on demand', () => {
-    let numScripts = 0;
-    cy.get('script').then(nodes => {
-      numScripts = nodes.length;
-    });

Review comment:
       nit: we should maybe update the test name to reflect the fact that we're testing the edit feature.

##########
File path: superset-frontend/cypress-base/cypress/integration/explore/AdhocFilters.test.ts
##########
@@ -26,10 +26,6 @@ describe('AdhocFilters', () => {
     cy.verifySliceSuccess({ waitAlias: '@postJson' });
   });
 
-  xit('Should not load mathjs when not needed', () => {
-    cy.get('script[src*="mathjs"]').should('have.length', 0);
-  });
-

Review comment:
       Note to other reviewers: we're fully removing `mathjs` as a dependency right now due to bundle size and compatibility problems with Webpack 5. Related PR removing `mathjs` from `superset-ui`: https://github.com/apache-superset/superset-ui/pull/1362 

##########
File path: superset-frontend/webpack.config.js
##########
@@ -141,17 +152,6 @@ if (!process.env.CI) {
   plugins.push(new webpack.ProgressPlugin());
 }
 
-// clean up built assets if not from dev-server
-if (!isDevServer) {
-  plugins.push(
-    new CleanWebpackPlugin({
-      dry: false,
-      // required because the build directory is outside the frontend directory:
-      dangerouslyAllowCleanPatternsOutsideProject: true,
-    }),
-  );
-}
-

Review comment:
       Is this no longer needed? I remember this having been a problem before causing lots of accumulated old bundles in the build directory.

##########
File path: superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx
##########
@@ -204,10 +204,25 @@ export default class AnnotationLayer extends React.PureComponent {
     return sources;
   }
 
-  isValidFormula(value, annotationType) {
+  isValidFormula(expression, annotationType) {

Review comment:
       bycatch: let's either rename this to `isInvalidFormula` or flip the return values around (this would be my preference).




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

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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