-
Notifications
You must be signed in to change notification settings - Fork 804
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
Migrate workbox-build to TypeScript #2867
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several linting errors and I added a couple of suggestions.
packages/workbox-build/src/lib/additional-manifest-entries-transform.ts
Outdated
Show resolved
Hide resolved
packages/workbox-build/src/lib/additional-manifest-entries-transform.ts
Outdated
Show resolved
Hide resolved
Thanks! Regarding the linting errors, I'm not going to be able to resolve the three instances of The other errors should be resolved now, though. |
Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
…nsform.ts Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
…nsform.ts Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry there was a typo in one of my previous suggestions, I added the fix.
Ack on the map undefined issue.
LGTM
packages/workbox-build/src/lib/additional-manifest-entries-transform.ts
Outdated
Show resolved
Hide resolved
…nsform.ts Co-authored-by: Adriana Jara <32825533+tropicadri@users.noreply.github.com>
R: @tropicadri
Fixes #2806, fixes #2477
There's obviously a lot going on in this PR. Sorry!
At a high level, this migrates
workbox-build
to TypeScript. (Following this PR, onlyworkbox-webpack-plugin
andworkbox-sw
are written in JavaScript.)The goal was to preserve functionality during this migration, and not require a major semver bump. The biggest change under the hood is swapping out
@hapi/joi
option validation for a two-part system that generates JSON schema based on our TypeScript definitions at build time, and then uses https://ajv.js.org/ to validate at runtime against that schema.@hapi/joi
is a bit more sophisticated in terms of complex runtime validation (e.g. ensuring that if one option is set, another option needs to also be set, making options mutually exclusive, etc.) so some of those relationships needed to be expressed at runtime via custom functions, meaning that the generated JSON schema are not 100% the source of truth.Many of the test changes across all the packages reflect the fact that validation now throws slightly different errors due to the above mentioned change.
There are other small changes sprinkled throughout this PR as well, based on things that I found necessary while working with the additional TypeScript code. Please flag anything that looks amiss!