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: next #4655

Merged
merged 66 commits into from
Jan 22, 2024
Merged

feat: next #4655

merged 66 commits into from
Jan 22, 2024

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Nov 26, 2022

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

We are preparing for the next major release.

TODO:

  • Drop Node v12
  • Drop webpack 4
  • Remove deprecated options
  • Resolve TODO wherever possible
  • Update to webpack-dev-middleware v6
  • remove magicHtml option

Breaking Changes

Additional Info

@codecov
Copy link

codecov bot commented Nov 26, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (540c438) 90.38% compared to head (7a3eab9) 92.07%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4655      +/-   ##
==========================================
+ Coverage   90.38%   92.07%   +1.68%     
==========================================
  Files          16       15       -1     
  Lines        1706     1577     -129     
  Branches      649      600      -49     
==========================================
- Hits         1542     1452      -90     
+ Misses        150      116      -34     
+ Partials       14        9       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@snitin315
Copy link
Member Author

@alexander-akait Should we remove the support of the compiler option as the first argument in this release?

// TODO: remove this after plugin support is published
if (/** @type {Compiler | MultiCompiler} */ (options).hooks) {
util.deprecate(
() => {},
"Using 'compiler' as the first argument is deprecated. Please use 'options' as the first argument and 'compiler' as the second argument.",
"DEP_WEBPACK_DEV_SERVER_CONSTRUCTOR"
)();

Also, I'm not sure what needs to be done in the following TODOs to resolve them:

// TODO improve for the next major version - we should store `web` and other targets in `compiler.options.environment`

// TODO maybe empty client

// TODO show warning about this

// TODO: do merge in the next major release

// TODO: do merge in the next major release

// TODO remove in the next major in favor `context` and `router` options

@alexander-akait
Copy link
Member

@snitin315 Let's rebase and I think we need finish it this month 😄 What we need to finish?

@snitin315
Copy link
Member Author

@alexander-akait only this #4655 (comment)

@alexander-akait
Copy link
Member

Should we remove the support of the compiler option as the first argument in this release?

Yeah, it will be pain, but we need to do it, our webpack-cli is ready for this?

// TODO improve for the next major version - we should store web and other targets in compiler.options.environment

We need to solve it on webpack side, because we don't have compiler.options.environment https://github.com/webpack/webpack/blob/main/lib/config/defaults.js#L928 and we can't know what env of developer, will be great to fix in on webpack side, just store options.environment in compiler.options.environment

// TODO maybe empty client
// TODO show warning about this

The first is a small bug, but I don't think somebody use it, the second just a warning improvement

// TODO: do merge in the next major release

Not sure what I want here 😃 Maybe we should merge default options with provided, so developer dont' need to seutp all of them, need investigate what is default and what is our to avoid massive regressions

// TODO remove in the next major in favor context and router options

It is very simple PR but and hard in the same time, http-proxy-middleware has this feature and you can achive it using router, but I am afraid we will break a lot, so let's generate a warning here, like "bypass is deprecated, please look at example of migration (link on examples/docs in http-proxy-middleware repo)"

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 19, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@snitin315
Copy link
Member Author

/easycla

@snitin315
Copy link
Member Author

// TODO improve for the next major version - we should store web and other targets in compiler.options.environment

We need to solve it on webpack side, because we don't have compiler.options.environment https://github.com/webpack/webpack/blob/main/lib/config/defaults.js#L928 and we can't know what env of developer, will be great to fix in on webpack side, just store options.environment in compiler.options.environment

@alexander-akait You mean we should add a new option environment in webpack config and it should be union of target and output.environment?

@snitin315
Copy link
Member Author

// TODO maybe empty client

// TODO maybe empty client

Do you mean if the user passed client: {}? Even then we normalize client options and it will be resolved to { webSocketURL: {}, overlay: true, reconnect: 10, logging: 'info' }

@alexander-akait
Copy link
Member

Do you mean if the user passed client: {}?

Yeah, developer can use it, we just need to normalize to our default value

@alexander-akait
Copy link
Member

@alexander-akait You mean we should add a new option environment in webpack config and it should be union of target and output.environment?

I think we need this https://github.com/webpack/webpack/blob/main/lib/config/defaults.js#L146 because we need to undestand what is enviroment, is it web or is it node is it something else

@alexander-akait
Copy link
Member

@snitin315 Also, I think let's remove magicHtml option, it is broken for modules, don't see who used it, also it decrease perf, because we need to check it on every request

@alexander-akait
Copy link
Member

@snitin315 Can you show me what is left, so I will plan our release and my work, thank you

@snitin315
Copy link
Member Author

@alexander-akait Following TODOs are pending, but we can postpone some.

// TODO improve for the next major version - we should store `web` and other targets in `compiler.options.environment`

// TODO maybe empty client

// TODO: we respect these options for all watch options and allow developers to pass them to chokidar, but chokidar doesn't have these options maybe we need revisit that in future

// TODO: do merge in the next major release

Other than this I will start writing a migration guide for v5 and update docs on webpack.js.org too.

@alexander-akait
Copy link
Member

@snitin315 Yeah, let's start to write migration guide, because they are just fixes 👍

@alexander-akait
Copy link
Member

@snitin315 Sorry for pings 😄 Any progress?

@snitin315
Copy link
Member Author

@alexander-akait Hi, I was away from home last week, I'll send PR for the migration guide tonight. In the meantime, let's not merge more deps PR in the master.

@alexander-akait
Copy link
Member

alexander-akait commented May 22, 2023

@snitin315 Thank you, yeah, agree 👍 (hope there are no critical bugs while we are preparing release 😄 )

@alexander-akait
Copy link
Member

alexander-akait commented May 28, 2023

Okay, I looked deeply on our TODOs, we need to finish

  • TODO: do merge in the next major release
  • TODO improve for the next major version - we should store web and other targets in compiler.options.environment

I will send PR in webpack for the latest, for the first we should need merge options with our defaults values (now we are override them)

@alexander-akait
Copy link
Member

@snitin315 Let's update webpack-dev-middleware to the latest version here too

@alexander-akait
Copy link
Member

@snitin315 Did we resolve all TODOs here? Because after updating webpack-dev-middleware I want to merge it to the master/main

@snitin315
Copy link
Member Author

snitin315 commented Jan 16, 2024

@alexander-akait Following TODOs are pending, maybe we can postpone these. I'll update webpack-dev-middleware.

// TODO improve for the next major version - we should store `web` and other targets in `compiler.options.environment`

// TODO maybe empty client

// TODO improve for the next major version - we should store `web` and other targets in `compiler.options.environment`

@alexander-akait
Copy link
Member

Looks, fine, we can improve them later, because it is not a blocker

@snitin315 snitin315 marked this pull request as ready for review January 21, 2024 04:36
@alexander-akait
Copy link
Member

Let's merge

@alexander-akait
Copy link
Member

@snitin315 Will you write the migration guide? Thank you

@alexander-akait alexander-akait merged commit 26aae7e into master Jan 22, 2024
65 of 78 checks passed
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.

4 participants