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

chore: move cli under lerna package #1225

Merged
merged 18 commits into from
Feb 17, 2020
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Feb 14, 2020

What kind of change does this PR introduce?

Refactor. This PR moves the cli code under a lerna package. This will help the symlinks between different packages and it will help the release process.

Did you add tests for your changes?
No. Tests should all pass after this refactor.

If relevant, did you update the documentation?
No. It should be done in later commits.

Summary
The cli should be under the lerna supervision. This is useful because it will help the creation of changelog, tagging, committing and publishing using one single tool.

Also, I changed the way the serve package retrieves the arguments from the CLI. Instead of referring with a relative path, it now import webpack-cli package (note the change in package.json of the cli):

"main": "./lib/webpack-cli.js"

And it exposes a getter (getCoreFlags) that are passed to the serve command.

Moved to yarn. It's essential to use the workspaces features so we can link locally all the packages together and we can test them seamlessly!

Does this PR introduce a breaking change?

It should not.

Other information

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@ematipico ematipico changed the title [WIP] chore: move cli under lerna package chore: move cli under lerna package Feb 14, 2020
Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

Looks great, but have left some suggestions.
Instead of naming sub package "webpack-cli" we should call it "@webpack-cli/core" as it will go good with the conventional naming of sub packages. Like @babel/cli, @webpack-cli/core, etc.

Also we should keep bin in the main package itself and spawn cli from "core" sub package.

packages/cli/package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ematipico
Copy link
Contributor Author

ematipico commented Feb 17, 2020

@rishabh3112

Instead of naming sub package "webpack-cli" we should call it "@webpack-cli/core" as it will go good with the conventional naming of sub packages. Like @babel/cli, @webpack-cli/core, etc.

We still need a package called webpack-cli because that's what we are going to publish and that's what the users are installing (npm i -D webpack-cli). We could have a core package but we still need the webpack-cli one.

Also we should keep bin in the main package itself and spawn cli from "core" sub package.

The main package won't be published on the registry (it has to be private). The cli folder (I will rename it webpack-cli in my next PR) is the actual CLI and that's where the binary has to be defined.

rishabh3112
rishabh3112 previously approved these changes Feb 17, 2020
Co-Authored-By: James George <jamesgeorge998001@gmail.com>
@ghost
Copy link

ghost commented Feb 17, 2020

There were the following issues with this Pull Request

  • Commit: b96009c
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@webpack-bot
Copy link

@ematipico Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@jamesgeorge007 Please review the new changes.

@ematipico ematipico merged commit 358651e into webpack:next Feb 17, 2020
@anshumanv
Copy link
Member

This is superb, makes it way easy to setup the project now that it leverages yarn workspaces. Kudos! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants