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

Feat: Allow flags to support the webpack-cli #628

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

info-arnav
Copy link

@info-arnav info-arnav commented Apr 8, 2024

Context

The webpack-bundle-analyzer has various options such as host which are accessible via flags with reference to webpack-cli#1838. In this PR we would be trying to support these flags in the webpack-cli, like below :

webpack build --analyze --analyze-no-open

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

And now let's use them here - https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/master/src/bin/analyzer.js#L27

there is an API - https://github.com/webpack/webpack/blob/main/lib/cli.js#L612
So:

  • you require options from this file
  • then use processArguments (don't forget to handle Problem)
  • then use .option(...) to create all options

@info-arnav
Copy link
Author

And now let's use them here - https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/master/src/bin/analyzer.js#L27

there is an API - https://github.com/webpack/webpack/blob/main/lib/cli.js#L612 So:

  • you require options from this file
  • then use processArguments (don't forget to handle Problem)
  • then use .option(...) to create all options

on it

src/bin/analyzer.js Outdated Show resolved Hide resolved
@valscion
Copy link
Member

valscion commented Apr 9, 2024

What are flags?

const {resolve, dirname} = require('path');
const { resolve, dirname } = require("path");
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 to have so much formatting changes here?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change this big amount of packages that GitHub even has trouble visualizing the changes in package-lock.json?

@alexander-akait
Copy link
Member

@valscion We need these flags to support webpack CLI - webpack/webpack-cli#1838, so you can use webpack build --analyze --analyze-host 127.0.0.1, so you don't need to have own CLI anymore (and can drop it in the future major release) and we will provide a better and unified API for our developers, less maintance problems for you in future too

@valscion
Copy link
Member

Thanks for the background information! It would indeed be valuable to allow this tool to be used easily through webpack CLI.

In order for me to eventually be able to review this PR, I'd need to see the least amount of changes possible. That includes:

  • Not doing any unnecessary formatting changes
  • Not touching dependencies in this PR (so no changes to package-lock.json)

And then we'd need these:

  • Test coverage in here to verify we don't regress this feature in the future
    • The best test coverage would be one that would also use the webpack CLI so that we know the interplay between this package and webpack CLI works as intended.
    • I'm open to hearing alternatives if said test coverage is too difficult to add
  • A PR description which outlines how this feature would is intended to work
  • A changelog entry
  • Maybe even a README.md change if such a change is warranted?

Also, I'd like to know whether this change will indeed still be backwards-compatible (that is, releasing this would be a minor version release and not a major release). If there is no other way than doing a major release then I'm also OK with that.

All in all, I do think that this feature would make sense — but I also have some wishes as the maintainer of this project before I can accept the changes. ☺️

@alexander-akait
Copy link
Member

@valscion Yeah, I will ping you when it will be ready to review

@valscion
Copy link
Member

Sounds nice! Thanks!

@info-arnav
Copy link
Author

@alexander-akait shall i proceed to write the tests ?

@info-arnav info-arnav changed the title feat: added flags Feat: Allow flags to support the webpack-cli Apr 13, 2024
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

3 participants