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-bundle-analyzer] webpack@5 compatibility #48857
[webpack-bundle-analyzer] webpack@5 compatibility #48857
Conversation
@kryops Thank you for submitting this PR! This is a live comment which I will keep updated. 2 packages in this PRCode ReviewsThis PR can be merged once it's reviewed. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 48857,
"author": "kryops",
"owners": [
"kryops",
"VladimirGrenaderov",
"maxbogus",
"peterblazejewicz",
"tkqubo",
"bumbleblym",
"bcherny",
"tommytroylin",
"mohsen1",
"jcreamer898",
"alan-agius4",
"dennispg",
"christophehurpeau",
"ZSkycat",
"johnnyreilly",
"rwaskiewicz",
"kuehlein",
"grgur",
"rubenspgcavalcante",
"andersk",
"ofhouse",
"danielthank",
"sasurau4",
"dionshihk",
"spamshaker"
],
"dangerLevel": "MultiplePackagesEdited",
"headCommitAbbrOid": "ceb69de",
"headCommitOid": "ceb69de4d181b5663f253092c8a1f35f9ef3294d",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastPushDate": "2020-10-16T17:08:08.000Z",
"lastCommentDate": "2020-10-20T15:09:29.000Z",
"maintainerBlessed": true,
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/48857/files",
"hasMergeConflict": false,
"authorIsOwner": true,
"isFirstContribution": false,
"popularityLevel": "Critical",
"newPackages": [],
"packages": [
"webpack-bundle-analyzer",
"webpack"
],
"files": [
{
"path": "types/webpack-bundle-analyzer/index.d.ts",
"kind": "definition",
"package": "webpack-bundle-analyzer"
},
{
"path": "types/webpack-bundle-analyzer/webpack-bundle-analyzer-tests.ts",
"kind": "test",
"package": "webpack-bundle-analyzer"
},
{
"path": "types/webpack/index.d.ts",
"kind": "definition",
"package": "webpack"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-10-16T18:01:54.000Z",
"firstApprovalDate": "2020-10-16T18:01:54.000Z",
"reviewersWithStaleReviews": [],
"approvalFlags": 2,
"isChangesRequested": false
} |
🔔 @VladimirGrenaderov @maxbogus @peterblazejewicz @tkqubo @bumbleblym @bcherny @tommytroylin @mohsen1 @jcreamer898 @alan-agius4 @dennispg @christophehurpeau @ZSkycat @johnnyreilly @rwaskiewicz @kuehlein @grgur @rubenspgcavalcante @andersk @ofhouse @danielthank @sasurau4 @dionshihk @spamshaker — please review this PR in the next few days. Be sure to explicitly select |
@kryops The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
Mentioning other webpack related definition authors to raise awareness so we're not all trying to solve the same problem at the same time. Sorry for the spam, please unsubscribe if this is irrelevant for you: @AGenson @andersk @AviVahl @bahlo @blaise-io @bumbleblym @ChaosinaCan @danpintara @Danscho @davecardwell @deevus @dsifford @dublicator @e-cloud @flying-sheep @FranklinWhale @garbetjie @Hotell @huan086 @iclanton @inglec-arista @j-f1 @JounQin @JuanJoseGonGi @karol-majewski @katyo @kgroat @maestroh @malj @maxbogus @monsonjeremy @mtraynham @nhardy @odnamrataizem @pd4d10 @peterblazejewicz @pine @playma256 @r3nya @remcohaszing @rynclark @Seally @skovy @tkqubo @use-strict @vajkayrene @VladimirGrenaderov @woitechen |
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. webpack-bundle-analyzer (unpkg)was missing the following properties:
webpack (unpkg)was missing the following properties:
as well as these 6 other properties...WebpackOptionsValidationError, dependencies, web, webworker, node, util |
Hi, Most of the exported names came from a time (webpack@2.x.x) where there were no types or annotation in the webpack repo itself. Changing all the names now makes no sense since this typings will be deprecated eventually. But when you find something that makes the upgrade to webpack@5.x.x easier and is a non-breaking change then you are welcome to add them. |
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? webpack-bundle-analyzer/v*Comparison details for webpack-bundle-analyzer/* 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. webpack/v*Comparison details for webpack/* 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
Ready to merge |
I just published |
I just published |
thanks! |
Fixes #48807
This is a bit of a tricky one: webpack@5's own types declare a
WebpackPluginInstance
interface, while the webpack@4 types /@types/webpack
maintained here have an abstractPlugin
class, which is used by type definitions for webpack plugins. There are plans to extend webpack@5's own types and maybe add aPlugin
alias there (see webpack/webpack#11630 and webpack/webpack#11698), but they are not in yet.I tried it the other way around and added a
WebpackPluginInstance
interface to the webpack@4 types defined here, which makes thewebpack-bundle-analyzer
types compatible with both@types/webpack
andwebpack@5
at its current state.Asking the maintainers of
@types/webpack
(who the Bot will hopefully ping): Do you think this is ok or should we rather wait until webpack aligns their own types with the existing ecosystem?Same goes with the
Stats
options, which are currently defined asany
in webpack@5: I copied them into the plugin's type definition for now. What should plugin type definitions do with types that webpack@5 currently does not offer or export?Did other webpack plugin definitions already add webpack@5 compatibility or is this the first attempt?
Please fill in this template.
npm test YOUR_PACKAGE_NAME
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition: