[DEV] Allow webpack-dev-server proxy to any destination #9296

ktmud · 2020-03-12T18:50:05Z

SUMMARY

One of the main pain points in developing Superset frontend features is the lack of testing data. Local installations often do not have enough examples conveniently set up to test different edge cases. Because of the multi-repo workflow with chart plugins, it also get complicated to set up an editable frontend building environment on remote servers.

This PR allows webpack-dev-server to proxy to any remote Superset web service, but at the same time replaces CSS/JS asset references in the response HTML with locally built chunk files that have source map and hot reload support. The remote Superset service must be running on a version later than this PR.

This allows developers to start the frontend server locally, but test with production/staging data served by a remote server. This setup makes it easier to debug edge cases all while maintaining the productivity of writing code in a local IDE.

The only caveat is that this solution may not work for some complex user authentication configurations that require OAuth callbacks or cross-domain redirects.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Using following command to proxy dev-server to a remote Superset service:

npm run dev-server --superset=http://superset.dev.example.com

Or use a local reverse proxy or SSH port forwarding to forward remote service tohttp://localhost:8088.

The page should load normally with locally built assets and JS hot reloading should work.

ADDITIONAL INFORMATION

N/A

REVIEWERS

cc @kristw @etr2460 @rusackas @john-bodley

ktmud · 2020-03-13T07:31:38Z

superset-frontend/webpack.proxy-config.js

+// make sure the manifest file exists
+fs.mkdirSync(path.dirname(MANIFEST_FILE), { recursive: true });
+fs.closeSync(fs.openSync(MANIFEST_FILE, 'as+'));
+fs.watch(MANIFEST_FILE, loadManifest);


Reload manifest every time there is a new build.

superset-frontend/webpack.proxy-config.js

superset/__init__.py

ktmud · 2020-03-13T07:44:43Z

superset/templates/superset/partials/asset_bundle.html

+{% macro js_bundle(filename) %}
+  {# HTML comment is needed for webpack-dev-server to replace assets
+     with development version #}
+  <!-- Bundle js {{ filename }} START -->


The downside is this adds such HTML comments even to the production mode. Can probably create a config flag DISABLE_ASSET_BUNDLE_WATERMARK to turn this off.

ktmud · 2020-03-13T22:44:06Z

superset-frontend/package.json

@@ -236,7 +236,8 @@
    "webpack-bundle-analyzer": "^3.4.1",
    "webpack-cli": "^3.1.1",
    "webpack-dev-server": "^3.1.14",
-    "webpack-sources": "^1.1.0"
+    "webpack-sources": "^1.1.0",
+    "yargs": "12 - 15"


Replace minimist with yargs as the latter has better API and is included by other packages anyway. Choose this wide range (12 -15) also because it is tested working with the latest version but other packages still depend on v12.

What other packages are depending on it? Do we have any script that depends on it?
Could we try to get this to 15?

jest-cli and webpack-dev-server uses 12, vega-lite uses 15. 15 is the latest version, the latest webpack-dev-server hasn't upgraded it yet.

Wow, just found out there is a bot that automatically upgrade the packages for you, plus migrating all the code. Maybe we can use it to replace dependabot in superset-ui and superset-ui-plugins.

jest-cli and webpack-dev-server uses 12, vega-lite uses 15. 15 is the latest version, the latest webpack-dev-server hasn't upgraded it yet.

Do those list yargs as peerDependencies or dependencies? npm allow nested dependencies so it may install multiple versions of yargs, which is inefficient to have multiple copies, but not too bad. IMO, it might be good to be specific for the script stability.

Renovate looks interesting.

They list it as dependencies. The range is specifically to avoid multiple copies. I think it's OK to keep this range for now as we didn't use it too extensively and we already know both v12 and v15 works for our code.

ktmud · 2020-03-13T22:45:03Z

superset-frontend/webpack.config.js

+      () => {
+        return proxyConfig;
+      },
+    ],


Hot reload borrowed from here.

kristw

Overall, awesome work! This should make testing with production data much easier!

Will be good to have another python-eye to check the python code.

superset-frontend/webpack.proxy-config.js

superset/__init__.py

kristw · 2020-03-13T23:21:59Z

superset-frontend/package.json

@@ -236,7 +236,8 @@
    "webpack-bundle-analyzer": "^3.4.1",
    "webpack-cli": "^3.1.1",
    "webpack-dev-server": "^3.1.14",
-    "webpack-sources": "^1.1.0"
+    "webpack-sources": "^1.1.0",
+    "yargs": "12 - 15"


What other packages are depending on it? Do we have any script that depends on it?
Could we try to get this to 15?

superset-frontend/webpack.proxy-config.js

kristw · 2020-03-14T01:55:11Z

superset-frontend/webpack.proxy-config.js

+}
+
+function toDevHTML(origHtml) {
+  let html = origHtml.replace(


could move this block of code below the loadManifest() block.

can use full word for clarity origHtml => originalHtml

I think origHtml is more in line with origRes used elsewhere in this file, otherwise should I rename origRes to originalResponse, henceforth res to response, and req to request as well?

That would be nice.

Updated the variable names!

codecov-io · 2020-03-14T23:15:02Z

Codecov Report

Merging #9296 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9296   +/-   ##
=======================================
  Coverage   59.08%   59.08%           
=======================================
  Files         374      374           
  Lines       12202    12202           
  Branches     2986     2986           
=======================================
  Hits         7209     7209           
  Misses       4814     4814           
  Partials      179      179

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1916a1...bc1d07c. Read the comment docs.

kristw

Thanks for addressing the comments. Just a few minor

kristw · 2020-03-16T18:12:26Z

superset-frontend/package.json

@@ -236,7 +236,8 @@
    "webpack-bundle-analyzer": "^3.4.1",
    "webpack-cli": "^3.1.1",
    "webpack-dev-server": "^3.1.14",
-    "webpack-sources": "^1.1.0"
+    "webpack-sources": "^1.1.0",
+    "yargs": "12 - 15"


jest-cli and webpack-dev-server uses 12, vega-lite uses 15. 15 is the latest version, the latest webpack-dev-server hasn't upgraded it yet.

Do those list yargs as peerDependencies or dependencies? npm allow nested dependencies so it may install multiple versions of yargs, which is inefficient to have multiple copies, but not too bad. IMO, it might be good to be specific for the script stability.

kristw · 2020-03-16T18:13:07Z

superset-frontend/webpack.proxy-config.js

+}
+
+function toDevHTML(origHtml) {
+  let html = origHtml.replace(


That would be nice.

kristw · 2020-03-16T18:20:51Z

superset-frontend/package.json

@@ -236,7 +236,8 @@
    "webpack-bundle-analyzer": "^3.4.1",
    "webpack-cli": "^3.1.1",
    "webpack-dev-server": "^3.1.14",
-    "webpack-sources": "^1.1.0"
+    "webpack-sources": "^1.1.0",
+    "yargs": "12 - 15"


Renovate looks interesting.

One of the pain points in developing Superset frontend code is the lack of testing data. Local installation often do not have enough examples setup to test all edge cases. This change allows `webpack-dev-server` to proxy to any remote Superset service, but the same time replaces frontend asset references in HTML with links to local development version. This allows developers to test with production data locally, tackling edge cases all while maintaining the productivity of editing the code locally.

ktmud · 2020-03-17T22:36:19Z

Tested again with a fresh installation. Didn't see any issues. I think this is ready to merge.

pull-request-size bot added the size/L label Mar 12, 2020

ktmud force-pushed the dev-server-proxy branch from 655ec9a to b4081b7 Compare March 13, 2020 07:01

ktmud commented Mar 13, 2020

View reviewed changes

ktmud force-pushed the dev-server-proxy branch 2 times, most recently from 2a299d8 to c993eac Compare March 13, 2020 21:45

ktmud changed the title ~~Allow webpack-dev-server proxy to any destination~~ [DEV] Allow webpack-dev-server proxy to any destination Mar 13, 2020

ktmud commented Mar 13, 2020

View reviewed changes

superset-frontend/webpack.config.js

() => {

return proxyConfig;

},

],

Copy link

Member Author

ktmud Mar 13, 2020 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hot reload borrowed from here.

ktmud force-pushed the dev-server-proxy branch from 501585f to 8c6c02e Compare March 13, 2020 23:37

kristw reviewed Mar 14, 2020

View reviewed changes

ktmud force-pushed the dev-server-proxy branch from 8c6c02e to 4b7a62e Compare March 14, 2020 22:56

ktmud force-pushed the dev-server-proxy branch from 19c3422 to 53ed851 Compare March 15, 2020 00:24

kristw approved these changes Mar 16, 2020

View reviewed changes

ktmud force-pushed the dev-server-proxy branch from bd95e43 to 4b11eba Compare March 17, 2020 20:42

kristw merged commit c36a7e3 into apache:master Mar 17, 2020

ktmud mentioned this pull request Mar 19, 2020

Build: optimize frontend build configs to improve superset-ui-plugin dev experience #9326

Merged

2 tasks

ktmud deleted the dev-server-proxy branch April 12, 2020 06:49

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEV] Allow webpack-dev-server proxy to any destination #9296

[DEV] Allow webpack-dev-server proxy to any destination #9296

ktmud commented Mar 12, 2020 •

edited

Loading

ktmud Mar 13, 2020

ktmud Mar 13, 2020

ktmud Mar 13, 2020

kristw Mar 13, 2020

ktmud Mar 14, 2020

ktmud Mar 14, 2020

kristw Mar 16, 2020

kristw Mar 16, 2020

ktmud Mar 16, 2020

ktmud Mar 13, 2020 •

edited

Loading

kristw left a comment

kristw Mar 13, 2020

kristw Mar 14, 2020

ktmud Mar 14, 2020

kristw Mar 16, 2020

ktmud Mar 16, 2020

codecov-io commented Mar 14, 2020 •

edited

Loading

kristw left a comment

kristw Mar 16, 2020

kristw Mar 16, 2020

kristw Mar 16, 2020

ktmud commented Mar 17, 2020

[DEV] Allow webpack-dev-server proxy to any destination #9296

[DEV] Allow webpack-dev-server proxy to any destination #9296

Conversation

ktmud commented Mar 12, 2020 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

kristw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 14, 2020 • edited Loading

Codecov Report

kristw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Mar 17, 2020

ktmud commented Mar 12, 2020 •

edited

Loading

ktmud Mar 13, 2020 •

edited

Loading

codecov-io commented Mar 14, 2020 •

edited

Loading