Skip to content

BREAKING CHANGE(server): get rid of conditions of Node version #2552

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

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Apr 30, 2020

  • 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?

Updated.

Motivation / Use-Case

Drop conditions of Node version.

Breaking Changes

Yes

Additional Info

// https://github.com/spdy-http2/node-spdy/issues/350
// https://github.com/webpack/webpack-dev-server/issues/1592
if (this.options.http2) {
// TODO: we need to replace spdy with http2 which is an internal module
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we wanna use http2 which is an internal module but we don't have enough time to implement because the interface isn't the same, so we use spdy for all Node versions because spdy supports 10, 12, and 14.

@hiroppy
Copy link
Member Author

hiroppy commented Apr 30, 2020

commitlint doesn't accept BREAKING CHANGE prefix lol

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #2552 into v4 will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v4    #2552      +/-   ##
==========================================
+ Coverage   93.36%   93.48%   +0.11%     
==========================================
  Files          34       34              
  Lines        1327     1320       -7     
  Branches      375      373       -2     
==========================================
- Hits         1239     1234       -5     
+ Misses         85       83       -2     
  Partials        3        3              
Impacted Files Coverage Δ
lib/Server.js 96.73% <100.00%> (+0.37%) ⬆️

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 f5498c9...4cc6dab. Read the comment docs.

@hiroppy
Copy link
Member Author

hiroppy commented Apr 30, 2020

#2553 must be merged first.

@hiroppy hiroppy marked this pull request as ready for review April 30, 2020 01:17
@alexander-akait
Copy link
Member

@hiroppy need rebase 👍

webpack_version: latest
node-10-canary:
node_version: ^10.13.0
node_version: ^10.20.1
Copy link
Member

Choose a reason for hiding this comment

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

Why we update version?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the latest version but you want to lock the v10.13.0? Anyway, http2 is supported over v10.10.0 so I'm fine either way.

https://nodejs.org/api/http2.html

Copy link
Member

Choose a reason for hiding this comment

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

@hiroppy azure and github actions by default install latest version if you use ^10.13

Copy link
Member Author

@hiroppy hiroppy Apr 30, 2020

Choose a reason for hiding this comment

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

yes I know but I don't know why you are sticking to v10.13.0. What are the benefits of fixing?

Copy link
Member

Choose a reason for hiding this comment

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

@hiroppy by default we support minimum LTS

Copy link
Member Author

@hiroppy hiroppy Apr 30, 2020

Choose a reason for hiding this comment

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

No, LTS is just v10, not v10.13. https://nodejs.org/ja/about/releases/

@hiroppy hiroppy force-pushed the feature/remove-version-condition branch from e0c3129 to 00f8422 Compare April 30, 2020 09:17
package.json Outdated
@@ -11,7 +11,7 @@
"client"
],
"engines": {
"node": ">= 6.11.5"
"node": ">= 10.20.1"
Copy link
Member

Choose a reason for hiding this comment

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

Return this to 10.13

Copy link
Member Author

Choose a reason for hiding this comment

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

I think 10.10 is better than 10.13 because this version supports http2 as a stable.

Copy link
Member

Choose a reason for hiding this comment

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

10.13 is LTS and we support LTS

Copy link
Member

Choose a reason for hiding this comment

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

Also webpack@5 support 10.13 as minimum, so we should support this range too

Copy link
Member Author

Choose a reason for hiding this comment

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

webpack@5 support 10.13 as minimum

ok, your answer is good for me. but 10.13 isn't minimum LTS.

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
Member Author

Choose a reason for hiding this comment

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

thank you for sharing this info.

@hiroppy hiroppy force-pushed the feature/remove-version-condition branch from 00f8422 to 4cc6dab Compare April 30, 2020 10:23
@hiroppy hiroppy requested a review from alexander-akait April 30, 2020 10:23
@hiroppy
Copy link
Member Author

hiroppy commented Apr 30, 2020

I've changed the commit message from BREAKING CHANGE to fix because CI is going to fail when using BREAKING CHANGE.

@hiroppy hiroppy merged commit ecab24b into v4 Apr 30, 2020
@hiroppy hiroppy deleted the feature/remove-version-condition branch April 30, 2020 11:32
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.

2 participants