Verify types.d.ts after generation #16703
Conversation
github-actions
bot
added
the
scripts
Interestingly this does not make anything fail?
github-actions
bot
added
the
schema
| @@ -262,7 +262,7 @@ | |||
| "errorMessage": { | |||
| "additionalProperties": "Feature names can only contain alphanumerical characters or the following symbols: _-$@" | |||
| }, | |||
| "tsType": "Record<string, Identifier> & {__compat?: CompatStatement};" | |||
| "tsType": "{[key: string]: Identifier} & {__compat?: CompatStatement};" | |||
There was a problem hiding this comment.
The TypeScript you're suggesting has broken imports of the data on Yari, because it's trying to map __compat to an Identifier type. Are you sure this is the right way to do this?
There was a problem hiding this comment.
This is just a manual definition of Record<string, Identifier>. It's semantically no different than Record<>, except it allows recursive reference. Note that this does not fail the test added by #16685.
There was a problem hiding this comment.
With this TypeScript type, however, we run into the following error on Yari:
The same issue occurs in a simplified test environment that uses the tsconfig.json from this project and includes an index.ts that just contains import bcd from "@mdn/browser-compat-data";.
The more I work with TypeScript, the more I hate it
There was a problem hiding this comment.
Tried to build Yari but it's broken in another way 🤔
$ npm run build
> @mdn/[email protected] build
> cross-env NODE_ENV=production node build/cli.js
/mnt/c/Users/Kagami/Documents/GitHub/yari/node_modules/fdir/src/api/walker.js:48
const needsSeperator = path[path.length - 1] !== sep;
^
builder/apiBuilder.js:18:10)
at allDocumentPathsAsTree (/mnt/c/Users/Kagami/Documents/GitHub/yari/content/document-paths.js:20:23)
at initAllDocumentsPathsTree (/mnt/c/Users/Kagami/Documents/GitHub/yari/content/document-paths.js:85:23)
at Object.<anonymous> (/mnt/c/Users/Kagami/Documents/GitHub/yari/content/document-paths.js:95:33)
at Module._compile (node:internal/modules/cjs/loader:1105:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Module._load (node:internal/modules/cjs/loader:827:12)
Node.js v18.1.0
npm notice
npm notice New minor version of npm available! 8.8.0 -> 8.12.2
npm notice Changelog: https://github.com/npm/cli/releases/tag/v8.12.2
npm notice Run npm install -g [email protected] to update!
npm notice
There was a problem hiding this comment.
Good old code problems left and right... 🙃
I have a pull request open that uses an updated version of BCD (which includes the error I screenshotted up above), if you'd like to try mdn/yari#6490 out?
There was a problem hiding this comment.
(__compat! is probably not a real fix, but anyway)
There was a problem hiding this comment.
You'll want to check the logs in the "Testing Yari" CI test: https://github.com/mdn/yari/runs/6906221112?check_suite_focus=true
(I'm aware of the other two issues reported by the tests, I plan to fix those at a later time, once this issue is resolved!)
There was a problem hiding this comment.
Hmm, I got what happened on Yari with your trial to fix:
It just made it to any, and that was hidden by skipLibCheck in Yari's tsconfig. 🙂
There was a problem hiding this comment.
I see, that makes sense -- so either way, it's broken...argh. This is honestly making me want to just give up on TypeScript altogether.
There was a problem hiding this comment.
Alright, I'm going to go ahead and merge this PR as-is with the reverted change, and then going to follow-up with updating the default TS export to cast to any first. I understand that's very bad to do and may result in unintended typecasts, but at this point, TypeScript users cannot use newer versions of BCD, including Yari.
Closes #16590
It seems it's even more broken since my report 😞Oh, actually it's not that much broken, the initial error is from #16685 and reverting it makes it green.