Skip to content

Add BuildProgressPlugin - #1011

Closed
Timer wants to merge 2 commits into
react:masterfrom
Timer:build-progress
Closed

Add BuildProgressPlugin#1011
Timer wants to merge 2 commits into
react:masterfrom
Timer:build-progress

Conversation

@Timer

@TimerTimer commented Nov 5, 2016

Copy link
Copy Markdown
Contributor

Reincarnation of #950. Please see it for details.

@TimerTimer mentioned this pull request Nov 5, 2016
@Timer

Timer commented Nov 5, 2016

Copy link
Copy Markdown
ContributorAuthor

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

@gaearongaearon added this to the 0.9.0 milestone Nov 30, 2016
@gaearon

Copy link
Copy Markdown
Contributor

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

fileName: 'asset-manifest.json'
})
}),
new BuildProgressPlugin()

Copy link
Copy Markdown
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.

@gaearongaearon left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

(It's really nice btw, great work)

@Timer
Timerforce-pushed the build-progress branch 4 times, most recently from cf4b001 to e4085c2CompareDecember 11, 2016 20:26
@Timer

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@Timer

Copy link
Copy Markdown
ContributorAuthor

Sure

@Timer
Timerforce-pushed the build-progress branch 2 times, most recently from 68e1eb7 to a9e6465CompareJanuary 23, 2017 20:38
@Timer

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

👍

@Timer

Copy link
Copy Markdown
ContributorAuthor

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

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

👍

@Timer

Timer commented Feb 8, 2017

Copy link
Copy Markdown
ContributorAuthor

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.

@TimerTimer modified the milestones: 0.10.0, 0.9.0Feb 8, 2017
@gaearongaearon modified the milestones: 0.11.0, 0.10.0, 0.10.1May 8, 2017
@gaearongaearon modified the milestones: 1.0.1, 1.0.xMay 19, 2017
@gaearon

Copy link
Copy Markdown
Contributor

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

@gaearongaearon closed this Jan 9, 2018
@locklockBot locked and limited conversation to collaborators Jan 20, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Timer@gaearon@facebook-github-bot