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: simplify init #2429

Merged
merged 106 commits into from
Mar 19, 2021
Merged

feat: simplify init #2429

merged 106 commits into from
Mar 19, 2021

Conversation

rishabh3112
Copy link
Member

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
WIP

If relevant, did you update the documentation?
Will do after implementation

Summary
Simplify the init feature to just scaffold configuration, without ast transformations.

Does this PR introduce a breaking change?
Yes

Other information
WIP

packages/generators/src/init-generator.ts Outdated Show resolved Hide resolved
packages/generators/src/init-generator.ts Show resolved Hide resolved
packages/generators/src/index.ts Outdated Show resolved Hide resolved
packages/generators/src/init-generator.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #2429 (6a5ad02) into master (4fccb73) will increase coverage by 18.21%.
The diff coverage is 93.10%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2429       +/-   ##
===========================================
+ Coverage   72.60%   90.82%   +18.21%     
===========================================
  Files          46       29       -17     
  Lines        2234     1449      -785     
  Branches      603      411      -192     
===========================================
- Hits         1622     1316      -306     
+ Misses        612      133      -479     
Impacted Files Coverage Δ
...s/generators/init-template/default/package.json.js 100.00% <ø> (ø)
packages/generators/src/types/index.ts 100.00% <ø> (ø)
packages/generators/src/init-generator.ts 92.30% <88.88%> (+1.19%) ⬆️
packages/generators/src/handlers/default.ts 94.11% <94.11%> (ø)
packages/generators/src/handlers.ts 100.00% <100.00%> (ø)
packages/generators/src/index.ts 100.00% <100.00%> (+2.38%) ⬆️
packages/generators/src/utils/scaffold-utils.ts 52.38% <0.00%> (-42.86%) ⬇️
packages/webpack-cli/lib/bootstrap.js 80.00% <0.00%> (-20.00%) ⬇️
...kages/webpack-cli/lib/utils/get-package-manager.js 57.69% <0.00%> (-3.85%) ⬇️
... and 1 more

Continue to review full report at Codecov.

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

@rishabh3112
Copy link
Member Author

Self Reminder: Add tests.

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.

Very good job, but I think we should still leave some questions:

  • dev server
  • CSS
  • babel
  • typescript
  • html-webpack-plugin

Because it is very popular and will be great to newbie + adding scripts - serve, build, dev, watch (maybe more or less). What do you think? Maybe I be some misleading. I mean we should not analyze webpack.config.js and just ask questions.

@rishabh3112
Copy link
Member Author

Also we need question for https://webpack.js.org/guides/asset-modules/ (please use type: 'asset'), but don't need to ask this if CSS or JS is true, otherwise url, new URL will not work, by default please use regexp for images and fonts

What question should we ask regarding this? I am thinking of Will you be using assets like images and fonts? currently.

@alexander-akait
Copy link
Member

@rishabh3112

Will you be using assets like images and fonts?

Sounds good and simple

@rishabh3112
Copy link
Member Author

Also we need question for https://webpack.js.org/guides/asset-modules/ (please use type: 'asset'), but don't need to ask this if CSS or JS is true, otherwise url, new URL will not work, by default please use regexp for images and fonts

I didn't understand what you meant in case we have CSS and JS rules, currently I am not adding asset module support when either of CSS and JS languages solutions are opted.

@alexander-akait
Copy link
Member

I didn't understand what you meant in case we have CSS and JS rules, currently I am not adding asset module support when either of CSS and JS languages solutions are opted.

If we don't add it, all url('./path/to/image.png') will not work. Solutions:

  • ask this always
  • ask this only when CSS is none

@alexander-akait
Copy link
Member

/cc @webpack/cli-team Need review, we will improve it more in the next PRs, basic work is done

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Member

@anshumanv anshumanv left a comment

Choose a reason for hiding this comment

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

Well donee 💯

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.

None yet

6 participants