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

Upgrade to webpack 4 #68

Merged
merged 12 commits into from
May 18, 2018
Merged

Upgrade to webpack 4 #68

merged 12 commits into from
May 18, 2018

Conversation

ydlamba
Copy link
Member

@ydlamba ydlamba commented May 15, 2018

Currently, not correctly working for production. (Work in Progress)

Fixes #53, #61.

@ydlamba ydlamba requested a review from domoritz May 16, 2018 16:01
package.json Outdated
"start": "webpack-dev-server --mode development --hot --inline --progress --open --color",
"build": "webpack --mode production --progress",
"clean": "rm -rf build",
"postbuild": "cp -rn public/* build",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a build folder and don't build directly into the public folder?

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see. Maybe better name it deploy.

Copy link
Member Author

Choose a reason for hiding this comment

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

/dist: "distribution", the compiled code/library
I think dist is like more standard.

@@ -3,10 +3,7 @@
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="theme-color" content="#325">

<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
Copy link
Member

Choose a reason for hiding this comment

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

No more favicon?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, html-webpack-plugin was loading it from public but now I think it's better to add the favicon in index.html itself. Adding it back.

@domoritz
Copy link
Member

domoritz commented May 16, 2018

  • Delete deploy.sh

@domoritz
Copy link
Member

The build works for me. Why do you say it doesn't work in production?

@domoritz
Copy link
Member

domoritz commented May 16, 2018

  • Reduce the number of chunks. 47 seems excessive unless I'm missing why they are necessary.

@domoritz
Copy link
Member

In Vega-Lite, we use rsync -r instead of cp -r. Can you find out what's better and use the better one everywhere?

@ydlamba ydlamba mentioned this pull request May 17, 2018
@ydlamba
Copy link
Member Author

ydlamba commented May 17, 2018

I have bundled everything in node_modules in a separate file vendors.chunk.js (no more 47 chunks).
Favicon is added back.
deploy.sh deleted.
Also, cp is replaced by rsync.
Now it's working in production too but few bundle sizes are large so webpack (in production) is showing size limit warnings.

@domoritz
Copy link
Member

I think we want a few chunks so that downloads can run in parallel but not 47.

What's the difference between cp and rsync?

@ydlamba
Copy link
Member Author

ydlamba commented May 17, 2018

There is not much difference between cp and rsync in copying local files. Just I have seen rsync as a preferred tool in many places.

Rsync is better since it will only copy only the updated parts of the updated file, instead of the whole file. It also uses compression and encryption if you want.

@ydlamba
Copy link
Member Author

ydlamba commented May 17, 2018

I have limited the maximum number of chunks to 5 (I also tried with 10 but it was creating small chunks thereafter, not much helpful). Try it if it works.

@ydlamba ydlamba changed the title [WIP] Upgrade to webpack 4 Upgrade to webpack 4 May 18, 2018
@domoritz domoritz merged commit 4190987 into master May 18, 2018
@domoritz domoritz deleted the ydl/webpack-4 branch May 18, 2018 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants