Skip to content
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.

Sign up for GitHub

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

Verify types.d.ts after generation #16703

Merged
merged 3 commits into from
Jun 17, 2022
Merged

Verify types.d.ts after generation #16703

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6bad754
Verify types.d.ts after generation
saschanaz Jun 16, 2022
7784932
Revert the schema change from #16685
saschanaz Jun 16, 2022
b222eca
Merge branch 'main' into exec-tsc
queengooborg Jun 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion schemas/compat-data.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@saschanaz saschanaz Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@queengooborg queengooborg Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this TypeScript type, however, we run into the following error on Yari:

Screen Shot 2022-06-16 at 15 34 11

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(__compat! is probably not a real fix, but anyway)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I got what happened on Yari with your trial to fix:

image

It just made it to any, and that was hidden by skipLibCheck in Yari's tsconfig. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},

"webextensions_identifier": {
Expand Down
2 changes: 2 additions & 0 deletions scripts/generate-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import fs from 'node:fs/promises';
import path from 'node:path';
import { execSync } from 'node:child_process';
import { fileURLToPath } from 'node:url';

import esMain from 'es-main';
Expand Down Expand Up @@ -123,6 +124,7 @@ const compile = async (
generateCompatDataTypes(),
].join('\n\n');
await fs.writeFile(destination, ts);
execSync('tsc ../types/types.d.ts', { cwd: dirname, stdio: 'inherit' });
};

if (esMain(import.meta)) {
Expand Down