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: init create webpack app #4167

Conversation

maverox
Copy link
Collaborator

@maverox maverox commented May 16, 2024

What kind of change does this PR introduce?

  • inits the create-webpack-app

Did you add tests for your changes?
yes
If relevant, did you update the documentation?

Summary

Does this PR introduce a breaking change?

Other information

@maverox maverox requested a review from a team as a code owner May 16, 2024 15:45
Copy link

linux-foundation-easycla bot commented May 16, 2024

} from "node-plop";

export type InitOptions = { template: string; force?: boolean };
export type LoaderOptions = { template: string };
Copy link
Member

Choose a reason for hiding this comment

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

RE: slack conv, you should make this generic. You see the pattern

Copy link
Collaborator Author

@maverox maverox May 18, 2024

Choose a reason for hiding this comment

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

Okay I will work on this soon,
I will definitely use the already established types and interfaces in @generators package as inspiration and tweak them.

This project has been created using **webpack-cli**, you can now run

```bash
npm run build
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what I am supposed to do here I would love if you can be a bit more specific,
all I can infer is you want me to change the run -> --run

Suggested change
npm run build
npm --run build

like this?
also I have read the section in the link provided

Copy link
Member

Choose a reason for hiding this comment

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

--run is not supported in v20 ,seems to be introduced in v22 only

add ejs for rendering logic using ejs templates in future
for compatibility with plopfile, as it throws error if it's a commonjs
file
- change templates to ejs templates
- implement ejs rendering logic in plopfile.ts
- remove helper function
- remove unnecessary comment
@maverox
Copy link
Collaborator Author

maverox commented May 18, 2024

@evenstensberg @snitin315 ,
I have resolved the problem with rendering
pushed the changes in the latest commits please have a look.

```js
const createWebpackApp = require("create-webpack-app");

// TODO: DEMONSTRATE API
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 promote the API usage? IMO, CWA interaction should be limited to CLI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking the same thing, will surely work on a better readme.

import { fileURLToPath } from "node:url";
import minimist from "minimist";
import { Plop, run } from "plop";
/* cSpell:disable */
Copy link
Member

Choose a reason for hiding this comment

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

Does this disable cspell for the whole file or just the next line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

section of code but I have found a better alternative

Suggested change
/* cSpell:disable */
/* cSpell:words plopfile */

"license": "MIT",
"repository": {
"type": "git",
"url": "https://github.com/webpack/create-webpack-app.git"
Copy link
Member

Choose a reason for hiding this comment

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

This is under the webpacki-cli repo

"watch": "tsc --watch"
},
"engines": {
"node": ">=14.15.0"
Copy link
Member

Choose a reason for hiding this comment

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

Let's support the minimum version as v18, we also have plans to drop support for v14 in other packages too.

Suggested change
"node": ">=14.15.0"
"node": ">=18.12.0"

Comment on lines 51 to 52
"peerDependencies": {},
"peerDependenciesMeta": {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"peerDependencies": {},
"peerDependenciesMeta": {}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed this in latest commit

import { NodePlopAPI } from "./types";
import { resolve } from "path";
import ejs from "ejs";
/* eslint-disable no-unused-vars */
Copy link
Member

Choose a reason for hiding this comment

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

Let's not disable the rule for the whole file, prefer to use //eslint-disable-next-line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed this in latest commits

message: "Enter your project name:",
default: "webpack-project",
validate(input, _) {
if (!input) {
Copy link
Member

Choose a reason for hiding this comment

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

A more strict check, in case the user just passes space.

Suggested change
if (!input) {
if (!input.trim()) {

},
{
type: "input",
name: "configPath",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: "configPath",
name: "projectPath",

Comment on lines 34 to 43
type: "input",
name: "entryPoint",
message: "Enter the entry point of your application:",
default: "src/index.js",
validate(input, _) {
if (!input) {
return "Entry point cannot be empty";
}
return true;
},
Copy link
Member

Choose a reason for hiding this comment

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

Since we are the ones generating templates we should have control over the entry point. We should not ask this question for scaffolding, users can refactor templates if they wish to post scaffolding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense.

- better input validation
- remove entrypoint prompt
- fix path issues
- add both index.js and index.ts
- fix bud in package.json template file
- fix the url
- bumped the required node from 14 -> 18
- removed empty peerDeps and peerMetaDeps fields
- fix cli entry point typo
@maverox
Copy link
Collaborator Author

maverox commented May 19, 2024

@snitin315 I have made all the changes you asked!
PTAL

Copy link

codecov bot commented May 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.52%. Comparing base (31f634a) to head (3fb7b98).
Report is 64 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (31f634a) and HEAD (3fb7b98). Click for more details.

HEAD has 36 uploads less than BASE
Flag BASE (31f634a) HEAD (3fb7b98)
40 4
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4167       +/-   ##
===========================================
- Coverage   90.18%   68.52%   -21.67%     
===========================================
  Files          22        9       -13     
  Lines        1702     1366      -336     
  Branches      491      427       -64     
===========================================
- Hits         1535      936      -599     
- Misses        167      430      +263     

see 18 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd1099a...3fb7b98. Read the comment docs.

- write whole command building and actions logic of commander in
src/index.ts for future scalability and type safety
- bin/cli.js now only contains importing this src/index.ts compiled
lib/index.js file.
- using dynamic action function to generate files based on choices
- change template structure
- using cross-spawn instead of normal spawn method
change stdio settings to incorporate each animations and spinners of the
underlying child process
made necessary changes based on the differences from the original cli
and its tests keeping original tests as base
earlier testing util spawn function was throwing error because of this.
.cspell.json Outdated
@@ -92,6 +92,7 @@
"testname",
"Tobio",
"tsbuildinfo",
"uzair",
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ohh I added this before I knew about Cspell:ignore

I will remove this in the next commit
"uzair" is my name actually

- remove projectName and the cumbersome projectPath/projectName
resolution
- adjust tests accordingly
- change template/**/package.json.tpl accordingly
- update readme accordingly
.eslintignore Outdated
@@ -11,3 +11,4 @@ test/build/config/error-commonjs/syntax-error.js
test/build/config/error-mjs/syntax-error.mjs
test/build/config/error-array/webpack.config.js
test/configtest/with-config-path/syntax-error.config.js
test/create-webpack-app/**
Copy link
Member

Choose a reason for hiding this comment

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

you can remove this now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

This project has been created using **webpack-cli**, you can now run

```bash
npm run build
Copy link
Member

Choose a reason for hiding this comment

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

--run is not supported in v20 ,seems to be introduced in v22 only

const ENABLE_LOG_COMPILATION = process.env.ENABLE_PIPE || false;
const isWindows = process.platform === "win32";

const hyphenToUpperCase = (name) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we already have this util in test-utils, you can just use that.

@@ -0,0 +1,426 @@
/* eslint-disable node/no-unpublished-require */
Copy link
Member

Choose a reason for hiding this comment

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

This file is not needed, it's already present in the test-utils directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed and reduced redundancy (ea92e56) significantly without affecting other suites.
checked locally by testing all but 1 test fails.

.aliases(["i", "n", "c", "create", "new"])
.description("Initialize a new Webpack project")
.argument("[projectPath]", "Path to create the project")
.option("-f, --force", "Skip the prompt and use the default values", false)
Copy link
Member

Choose a reason for hiding this comment

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

we need to add --template option too. The default value would be 'default'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it's okay I would like to do this change along with the react template and generator in a seperate PR.

- change dependencies for each prompt according to init command
reference
- change default for --force flag to appropriate values in index.ts as
well as plopfile.ts
- change conditional file generation for javascript langType
…/test-utils.js

- earlier this file was an exact copy with a line changed
- now it imports all the utility function and only extends run**()
functions with path value pointing to create-webpack-cli
- change snapshots according to new dependencies and defaults
- change the import in create-webpack-app.test.js
@snitin315 snitin315 changed the base branch from master to feat/create-webpack-app-package July 10, 2024 05:03
@snitin315 snitin315 merged commit 0986f40 into webpack:feat/create-webpack-app-package Jul 10, 2024
9 of 47 checks passed
snitin315 added a commit that referenced this pull request Jul 10, 2024
@maverox maverox deleted the feat/init-create-webpack-app branch July 12, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants