Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure webpack-dev-server #5786

Merged
merged 13 commits into from
Sep 4, 2018
Merged

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Aug 31, 2018

Goodbye, reload button! Instead of running npm run dev,

npm run dev-server

will start webpack dev server that wrap flask server at localhost:9000 and reload the page automatically (plus hot module replacement) when you change the js code and save.

Basically, you will go to http://localhost:9000 instead of http://localhost:8088 and after that, watch your browser refresh right after saving a file.

Advanced configuration

By default, webpack-dev-server is configured for flask running at port 8088.

If you start flask server at another port (e.g. 8081), you have to pass an extra argument
supersetPort to webpack-dev-server

npm run dev-server -- --supersetPort=8081

You can also specify port for webpack-dev-server

npm run dev-server -- --port=9001
# or with both dev-server port and superset port
npm run dev-server -- --port=9001 --supersetPort=8081

This will start dev server at port 9001 instead

p.s. Hot Module Replacement does not fully work with React yet. It will reload the entire page instead of reloading only part of the page. This needs react-hot-loader, which I can work on that in a follow-up PR.

@williaster @graceguo-supercat @hughhhh @conglei @xtinec

@codecov-io
Copy link

codecov-io commented Aug 31, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5786   +/-   ##
=======================================
  Coverage   63.75%   63.75%           
=======================================
  Files         364      364           
  Lines       23106    23106           
  Branches     2579     2579           
=======================================
  Hits        14731    14731           
  Misses       8360     8360           
  Partials       15       15

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 5e3f833...c8188dd. Read the comment docs.

@@ -11,6 +14,54 @@ const BUILD_DIR = path.resolve(__dirname, './dist');

const isDevMode = process.env.NODE_ENV !== 'production';
Copy link
Contributor

@williaster williaster Aug 31, 2018

Choose a reason for hiding this comment

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

one comment here is that I may have broken this flag when moving to webpack 4 which introduced the concept of mode=production/development (I was trying to use redux dev tools recently and it wasn't working locally, so I thought of this). I thought that also set a NODE_ENV (because webpack 4 was supposed to be all about smart defaults) but it seems like that might not be true.

So I think we either need to

  • reference the mode variable (ie argv.mode === 'production')
    OR
  • explicitly define the NODE_ENV in our npm webpack scripts (NODE_ENV=production webpack ...)

I can fix this in a separate PR as well if you prefer because it might have implications for the other logic in this file that relies on isDevMode (which I think is always true right now)

@williaster
Copy link
Contributor

This is going to be so great! Thanks for adding this! 🚀🚀🚀

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM! sorry about the node env issue, thanks for tweaking that to work. Interesting about the MiniCssExtractPlugin, I guess it's been in prod for a while, their documentation said to use style-loader for prod but seems okay.

@kristw
Copy link
Contributor Author

kristw commented Sep 1, 2018

@williaster Somehow using style-loader doesn't work as intended so I will leave that work (switching to style-loader for production) for another PR. At least this will ensure the same behavior with current production.

In my previous projects I use style-loader with ExtractTextPlugin but not sure what would be best for production here.

@@ -147,6 +147,7 @@ const config = {
'react/lib/ReactContext': true,
},
plugins,
devtool: isDevMode ? 'cheap-module-eval-source-map' : false,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@graceguo-supercat graceguo-supercat merged commit eb41756 into apache:master Sep 4, 2018
@kristw kristw deleted the kristw-webpack branch September 5, 2018 18:52
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
* setup devserver with correct proxy

* dev server wroking with written manifest

* add comments

* Change webpack to default port 9000 and minor js formatting

* Use hash in development.

* write to disk = true

* Take ports as argument for dev-server

* update instructions

* update instructions

* add devtools

* use mode instead of NODE_ENV

* use minicssextract for prod (like before)

* remove empty line
youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018
* setup devserver with correct proxy

* dev server wroking with written manifest

* add comments

* Change webpack to default port 9000 and minor js formatting

* Use hash in development.

* write to disk = true

* Take ports as argument for dev-server

* update instructions

* update instructions

* add devtools

* use mode instead of NODE_ENV

* use minicssextract for prod (like before)

* remove empty line

(cherry picked from commit eb41756)
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* setup devserver with correct proxy

* dev server wroking with written manifest

* add comments

* Change webpack to default port 9000 and minor js formatting

* Use hash in development.

* write to disk = true

* Take ports as argument for dev-server

* update instructions

* update instructions

* add devtools

* use mode instead of NODE_ENV

* use minicssextract for prod (like before)

* remove empty line
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants