-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
dont throw on 404s when fetching templates from iana #355
base: master
Are you sure you want to change the base?
Conversation
We don't have tests for the update script Im realizing |
@@ -94,7 +94,7 @@ async function addTemplateData (data, options) { | |||
return | |||
} | |||
|
|||
let res = await got('https://www.iana.org/assignments/media-types/' + data.template) | |||
let res = await got(`https://www.iana.org/assignments/media-types/${data.template}`, { throwHttpErrors: false }) |
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.
Actually we don't support new ES6+ syntax based on the CI: https://github.com/jshttp/mime-db/blob/master/.github/workflows/ci.yml#L13
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.
This script has es6 via async/await and let already, so the template string isn't a breaking introduction.
Interesting wrinkle here though, in that these scripts are part of the build tooling and we don't document the node version requirement for development/building. v11 of got has engines set to 10.19.0
https://github.com/sindresorhus/got/blob/1f6ac4597b797e6fe760a7dc11a3db8bf298aa94/package.json#L10C1-L11C1
I set up the automation PR that runs the build step on cron to use LTS node, so es6 is fine here.
Is this superseded by #352? or still relevant? |
They both will solve the 404 problem, and if we decide to switch to undici then this pr is not needed |
I prefer switching. Ideally at some point most of |
workaround for #345
When the update script was refactored to use
got
in 92715b3 it didn't factor in thatgot
throws on non success http status codesSo the built in 404 handling in the
addTemplateData
function will never run and we exit early on the unhandled promise rejection.A workaround is to set
throwHttpErrors: false
when fetchingBut the script needs a look over, since the assumptions about the http client's behavior are not valid anymore.
Why Now?
there were deprecated unofficial (extension) mime types which are 404ing now,
image/x-emf, image/x-wmf
I don't see when they went standard as
image/emf, image/wmf
, or when the extension versions started 404ing