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.

Sign up for GitHub

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

Jump to bottom

Add BuildProgressPlugin #1011

Closed
wants to merge 2 commits into from
Closed

Add BuildProgressPlugin #1011

wants to merge 2 commits into from

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Nov 5, 2016 •
edited
Loading

Reincarnation of #950. Please see it for details.

@Timer Timer mentioned this pull request Nov 5, 2016
Closed
@Timer
Copy link
Contributor Author

Timer commented Nov 5, 2016

@gaearon

require('webpack').ProgressPlugin should be fine. Peer dependency was only removed because now that we declare compat with Webpack 2, it will be annoying to bump it every few weeks. If npm provided a way to declare compat with all beta versions, I would've kept it.

But we are assuming specific plugins in react-dev-utils rely on Webpack. It's fine to import anything from it if these things are supported in 1.x and 2.x.

Are you saying that I should remove the peer dependency of webpack and trust that it is there at runtime? As of now, I used the semver matching from 4cd9fd9.

@gaearon gaearon added this to the 0.9.0 milestone Nov 30, 2016
@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

Are you saying that I should remove the peer dependency of webpack and trust that it is there at runtime?

Yes. (It's not as bad as it sounds, this plugin is useless without webpack anyway.)

gaearon
gaearon reviewed Dec 5, 2016
View reviewed changes
packages/react-scripts/config/webpack.config.prod.js Outdated
@@ -257,7 +258,8 @@ module.exports = {
// having to parse `index.html`.
new ManifestPlugin({
fileName: 'asset-manifest.json'
})
}),
new BuildProgressPlugin()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a small comment here explaining what it does. "Displays a progress bar during the build" is a fine way to put it.

gaearon
gaearon suggested changes Dec 5, 2016
View reviewed changes
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Remove peerDep + add a comment.

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

(It's really nice btw, great work)

@Timer Timer force-pushed the build-progress branch 4 times, most recently from cf4b001 to e4085c2 Compare December 11, 2016 20:26
@Timer
Copy link
Contributor Author

Timer commented Dec 11, 2016

Removed the peer dep, is this ok behavior though for when someone installs this for their own use?

gaearon
gaearon reviewed Dec 11, 2016
View reviewed changes
packages/react-dev-utils/BuildProgressPlugin.js Outdated

let ProgressPlugin;
if (__dirname.indexOf(path.join('packages', 'react-dev-utils')) !== -1) {
ProgressPlugin = require('../react-scripts/node_modules/webpack').ProgressPlugin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? Seems fishy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Webpack doesn't exist in any parent directory during development, only a sister directory (which node's module resolution doesn't traverse).
For development in the monorepo to work, we need a hack. I chose to do it this way, though we could accomplish it however.

tl;dr without it: Error: Cannot find module 'webpack'

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add it to devDependencies?

@gaearon
Copy link
Contributor

gaearon commented Dec 11, 2016

is this ok behavior though for when someone installs this for their own use?

Yes it’s fine. Peer deps are often more trouble than they’re worth, and until Webpack 2 is out of beta, they are definitely a pain since semver authors refuse to have a way to refer to non-stable versions via a range.

@gaearon
Copy link
Contributor

gaearon commented Jan 23, 2017

Ping, would you like to make the requested changes so we can get it in?

@Timer
Copy link
Contributor Author

Timer commented Jan 23, 2017

Sure

@Timer Timer force-pushed the build-progress branch 2 times, most recently from 68e1eb7 to a9e6465 Compare January 23, 2017 20:38
@Timer
Copy link
Contributor Author

Timer commented Jan 23, 2017

@gaearon pending CI, should be good -- though I'm not sure if I'm happy with the quality of progress the ProgressPlugin returns.
Also, it makes the CI log very dirty. Use your discretion or maybe we should explore alternatives to both outlets (progress plugin and progress package) (or submit PRs to webpack to make progress % better).

Maybe we should shut off the progress bar when not in an interactive terminal?

@gaearon
Copy link
Contributor

gaearon commented Jan 23, 2017

Maybe we should shut off the progress bar when not in an interactive terminal?

👍

@Timer
Copy link
Contributor Author

Timer commented Jan 24, 2017

@gaearon turns out travis is tty. I ended up checking for process.env.CI.

gaearon
gaearon reviewed Jan 24, 2017
View reviewed changes
packages/react-dev-utils/BuildProgressPlugin.js Outdated

function BuildProgressPlugin() {
if (process.env.CI) return new ProgressPlugin(function handler(percentage, msg) {
// noop
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: please use curly form of If statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

gaearon
gaearon reviewed Jan 24, 2017
View reviewed changes
packages/react-dev-utils/BuildProgressPlugin.js Outdated
width: 25
});
return new ProgressPlugin(function handler(percent, msg) {
if (percent === 1) msg = 'completed';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, please always use multiline if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@Timer
Copy link
Contributor Author

Timer commented Feb 8, 2017

Let's hold off on this until 0.10.0 and hope webpack's progress reporting got better. If not we can adjust it to just show current step / steps remaining.

@Timer Timer modified the milestones: 0.10.0, 0.9.0 Feb 8, 2017
@gaearon gaearon modified the milestones: 0.11.0, 0.10.0, 0.10.1 May 8, 2017
@viankakrisna viankakrisna mentioned this pull request May 17, 2017
Closed
@gaearon gaearon modified the milestones: 1.0.1, 1.0.x May 19, 2017
@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Closing as stale. Maybe worth revisiting if the tracking is accurate.

@gaearon gaearon closed this Jan 9, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon requested changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
1.0.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Timer @gaearon @facebook-github-bot