-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add mdn-bcd-collector update script #19971
Conversation
After speaking with @foolip I've made this as a first step in reducing tech debt in the update script by moving it into browser-compat-data. I tried to keep the number of changes low to the script itself low. There will be a lot of steps to do after merging this PR that we can list in #19868 or its own issue. |
package.json
Outdated
@@ -61,16 +62,20 @@ | |||
"eslint-plugin-unicorn": "^47.0.0", | |||
"fast-json-stable-stringify": "~2.1.0", | |||
"fdir": "~6.0.1", | |||
"fs-extra": "11.1.1", |
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.
I think this is only used for fs.readJson()
. Can you get rid of it and just use plain fs and JSON.parse()
like other existing code in BCD?
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.
As part of this I'm going to make reading the json files serial like what is done in index.ts
. update
at the moment reads the json in parallel. Since it is as of this time 1455 json files, some systems may run in EMFILE or ENFILE errors opening that many files at one time.
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.
Makes sense, I guess @queengooborg and I never ran into that.
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.
@mzgoddard the fs-extra dependency is still here. Can it be removed now?
This pull request has merge conflicts that must be resolved before it can be merged. |
I resolved conflicts and tested this script vs the one in GooborgStudios/mdn-bcd-collector. There were no differences between the changes both scripts made. Here is a gist with the diffs of each: https://gist.github.com/mzgoddard/b1ff3687630db9c6cf05579fb8413674 |
Brilliant, thank you @mzgoddard! I'll go ahead and merge this before there are more conflicts. |
Oh no, the Node LTS CI job is failing. @mzgoddard can you check if it's caused by these changes? |
@foolip I checked out the CI job. Its an error from
I don't think its related to the change in the PR. I think its a flaky error. If you re-run the job I think it will pass. (I don't have the permission needed to re-run.) |
@mzgoddard I re-ran the job but it's still failing. Unless it's also failing in other recent PRs I guess it was caused by the changes here? |
This pull request has merge conflicts that must be resolved before it can be merged. |
To help reduce potential for merge conflicts, we may want to split this down into even smaller chunks. For example, the UA parser I think could be its own PR (and I have some change requests regarding it specifically that would help turn it into a useful utility). |
I found the cause of the issue in CI testing. I made a mistake on one of the |
I'll make a PR with just UA parser and its test file. (That may not reduce merge conflicts by much since UA parser has its own dependency, |
This pull request has merge conflicts that must be resolved before it can be merged. |
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.
Thank you @mzgoddard for sorting out the CI issues and sending #20210 separately. I'll go ahead and merge this now.
Summary
Add update script from https://github.com/GooborgStudios/mdn-bcd-collector. Add needed utilities (
logger
andua-parser
) and npm dependencies.Test results and supporting details
I ran the update script locally with https://github.com/GooborgStudios/mdn-bcd-collector and https://github.com/GooborgStudios/mdn-bcd-results/tree/3af0b0ad1e7fd53098f266c80387e359288e21ad.
Running once completes without throwing an error.
Running a second time throws an error.
Error
I made a number of changes to the script:
mdn-bcd-collector/lib/constants.ts
toupdate
scriptmdn-bcd-collector/types/types.ts
toupdate
scriptArray<string, string, string, TestResultValue>
inOverrides
with[string, string, string, TestResultValue]
ManualOverride = [string, string, string, TestResultValue]
Array.isArray
inoverrides.filter
as(item: unknown) => item is ManualOverride
MDN_COLLECTOR_DIR
for loading overrides filepath.join(MDN_COLLECTOR_DIR, 'custom/overrides.json'),
Dependencies added to package:
fs-extra
klaw
ua-parser-js
winston
@google-cloud/logging-winston
Related issues
#19868
#18514