feat: migrate workbox-webpack-plugin to typescript #2882
Conversation
|
@roikoren755 Thank you for this, sorry for the late reply we've been doing some upgrading of TS lint and as you saw workbox-build migration to TS, I'll take a look at the PR and help where I can :) |
|
This is far from ready, but I wanted to share the updates.
Let me know if you have any questions, thank you for your patience with this one. |
passing tests for wb wpp cleaning imports
tropicadri
force-pushed
the
workbox-webpack-plugin-ts
branch
from
33df7d5
to
dfcc568
Compare
…orkbox into workbox-webpack-plugin-ts
tropicadri
force-pushed
the
workbox-webpack-plugin-ts
branch
from
4a4dac9
to
fa26432
Compare
|
Thanks! I'll take a look at this soon. |
There was a problem hiding this comment.
Thanks, @tropicadri! I have a few comments to go over once you're back in the office, but generally speaking, this looks good!
|
|
||
| if (sourcemapAssetName) { | ||
| _generatedAssetNames.add(sourcemapAssetName); | ||
| const sourcemapAsset = compilation.getAsset(sourcemapAssetName); | ||
| const {source, map} = await replaceAndUpdateSourceMap({ | ||
| jsFilename: config.swDest, | ||
| originalMap: JSON.parse(sourcemapAsset.source.source()), | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
What's the error here? If possible, could you use the form of the comment that disables a specific rule rather than all rules, so that it's clearer what's being worked around?
https://eslint.org/docs/user-guide/configuring/rules#using-configuration-comments
There was a problem hiding this comment.
I fixed the ones that didn't have the issue
packages/workbox-webpack-plugin/src/lib/get-manifest-entries-from-compilation.ts
Outdated
Show resolved
Hide resolved
| if (condition({asset, compilation})) { | ||
| return true; | ||
| } | ||
| return condition(asset.name); |
There was a problem hiding this comment.
I wonder if this is something that changed between webpack v4 and v5. I do see some v5 types that imply that conditions take a single string (and https://webpack.js.org/configuration/module/#condition sort of implies that too), but at one point I thought we needed to pass in {asset, compilation}... I am having trouble figuring out why that is, though.
But, since we aim to make this a backwards—compatible release, I feel like we need to keep the old method call here, and move to the "correct" signature of (value: string) => boolean as a breaking change in Workbox v7.
There was a problem hiding this comment.
I had to do some "anying" to keep it the way it was before, it doesn't seem to have changed a lot: https://v4.webpack.js.org/configuration/module/#condition, I am not sure why we are passing {asset, compilation} it seems that would invoke the case where compilation receives an object and all the properties should match, but I have no idea why it is implemented this way...so we'll keep it and change in v7
| @@ -159,14 +181,14 @@ function filterAssets(compilation, config) { | |||
| } | |||
|
|
|||
| // Next, check asset-level checks via includes/excludes: | |||
| const isExcluded = checkConditions(asset, compilation, config.exclude); | |||
| const isExcluded = checkConditions(asset, config.exclude); | |||
There was a problem hiding this comment.
See previous comment—we might need to keep the old signature for backwards compatibiltiy.
packages/workbox-webpack-plugin/src/lib/get-manifest-entries-from-compilation.ts
Show resolved
Hide resolved
tropicadri
force-pushed
the
workbox-webpack-plugin-ts
branch
from
3b2f9d0
to
d115025
Compare
There was a problem hiding this comment.
🥳
Thanks so much to @roikoren755 for kicking this off, and @tropicadri for finishing it up!
Prior to filing a PR, please:
gulp build && gulp lint testpasses locally.R: @jeffposnick @tropicadri
Fixes #2478
Description of what's changed/fixed.
This is a work in progress.
Couldn't get tests to run locally, and there are some
@ts-ignorecomments littered about in places that import things fromworkbox-buildorworkbox-google-analyticsthat I couldn't quite figure out.I think getting this package to use the new TS version of
workbox-buildwould get it almost ready to merge 🤞.Feel free to push changes, or point me in the right direction to fix things :)