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

[webpack-hot-middleware] Separating client and middleware options into 2 separate types #43625

Merged

Conversation

saboya
Copy link
Contributor

@saboya saboya commented Apr 4, 2020

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/webpack-contrib/webpack-hot-middleware#config
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

@saboya
Copy link
Contributor Author

saboya commented Apr 4, 2020

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Apr 4, 2020
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Apr 4, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 4, 2020

@saboya Thank you for submitting this PR!

🔔 @bumbleblym @icylace @chrisabrams @iliyaZelenko - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Apr 9, 2020
@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Apr 9, 2020
@typescript-bot
Copy link
Contributor

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@johanbaaij
Copy link

@saboya, should this have been a new major version?

@kevinmarrec
Copy link

kevinmarrec commented Apr 11, 2020

@johanbaaij Well the issue is that they keep in sync at least minor version between the package and its types. Which means they can only do patch releases on types.

Still that's bit boring :

"@types/webpack-hot-middleware": "^2.25.0"  // Works since 4 months

And implementing breaking type changes in 2.25.1 just break all projects that were importing options through :

import { Options as WebpackHotMiddlewareOptions } from 'webpack-hot-middleware'

@saboya
Copy link
Contributor Author

saboya commented Apr 11, 2020

@johanbaaij and @kevinmarrec: The problem is that passing client options to the middleware never worked anyway. So I'd rather let applications "break" to let developers know that what they're doing is... well, doing nothing.

@kevinmarrec
Copy link

@saboya Client options weren't the problem here. You changed main middleware options interface name, breaking the import even for people that never cared about client thing.

@saboya
Copy link
Contributor Author

saboya commented Apr 11, 2020

But it would break for anyone that did care about the client thing. And it wouldn't be an issue anyone unless you updated the types / didn't pin the version.

I see your point here about maintaining compatibility, and you can make a PR about it if you want. I think we can agree to disagree on this one.

@kevinmarrec
Copy link

kevinmarrec commented Apr 11, 2020

I'm totally fine with the changes, my only concern was having a "breaking" change on a patch version release. But as the types version are tied to the package itself version, it couldn't have been other way than patch version, so according this, there wasn't just ideal solution.

Also worth to mention that a "breaking" type is nothing compared to a "breaking" package feature. The types are all good now and let's stick with it.

@oneiho-safie
Copy link

[1m[31mERROR in /app/node_modules/@nuxt/types/config/build.d.ts
ERROR in /app/node_modules/@nuxt/types/config/build.d.ts(14,10):
14:10 Module '"../../../@types/webpack-hot-middleware"' has no exported member 'Options'.
    12 | import { BundleAnalyzerPlugin } from 'webpack-bundle-analyzer'
    13 | import { Options as WebpackDevMiddlewareOptions } from 'webpack-dev-middleware'
  > 14 | import { Options as WebpackHotMiddlewareOptions } from 'webpack-hot-middleware'

I has error when deploy in Jenkins. Useing Nuxt v2.x.
but build success in my local. need help.

@kevinmarrec
Copy link

@oneiho-safie
It means you don't have same dependencies on the two.
Nuxt types have been fixed on last packages releases.

You need to somehow update accordingly your dependencies against Jenkins

@oneiho-safie
Copy link

@kevinmarrec thx! I fix my package.json
"nuxt": "^2.0.0" to "nuxt": "^2.12.2" then deploy success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts). Unmerged The author did not merge the PR when it was ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants