-
Notifications
You must be signed in to change notification settings - Fork 52
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
feat: Improve type inference for oEmbed type in TypeScript #113
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.
Thanks this looks great
I don't have time to look at the failing tests right now though @r4ai . If you can get them passing i'll merge.
const oEmbed = | ||
result.oEmbed?.type === "video" ? result.oEmbed : (result.oEmbed as never); |
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.
If the oEmbed type is "link", width and height do not exist, so it must first be checked that the oEmbed type is "video".
Also, with only expect(result.oEmbed?.type).toEqual("video")
, TypeScript cannot infer that the type of result.oEmbed
is OEmbedVideo
. Therefore, the following type guard is additionally written.
const oEmbed = result.oEmbed?.type === "video" ? result.oEmbed : (result.oEmbed as never)
At this line, Since we have already checked that result.oEmbed?.type === "video"
in the above line, the "else" part is regarded as never
.
Thanks for the review! In the local environment, I could confirm that the test passed successfully, with Node.js version v21.6.1. $ npm run test
> unfurl.js@5.1.0 test
> npm run lint && port=9000 jest --verbose --runInBand --coverage --coveragePathIgnorePatterns '/test/'
> unfurl.js@5.1.0 lint
> eslint . --ext .ts
ts-jest[versions] (WARN) Version 4.8.4 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=2.7.0 <4.0.0). Please do not report issues in ts-jest if you are using unsupported versions.
(node:172689) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
PASS test/oembed/test.ts
✓ should noop and not throw for wrong content type (21ms)
✓ width/height should be numbers (10ms)
✓ should decode entities in OEmbed URL (10ms)
✓ should prefer fetching JSON oEmbed (8ms)
✓ should upgrade to HTTPS if needed (9ms)
✓ should build oEmbed from JSON (7ms)
✓ should build oEmbed from XML (7ms)
✓ should build oEmbed from XML with CDATA (10ms)
PASS test/encoding/test.ts
✓ should detect GB2312 charset (HTML 4) and convert to UTF-8 (24ms)
✓ should detect GB2312 charset (HTML 5) and convert to UTF-8 (2ms)
✓ should detect EUC-JP charset (HTML 5) and convert to UTF-8 (7ms)
PASS test/twitter_card/test.ts
✓ should build players[] (6ms)
✓ should build images[] (5ms)
✓ should build apps[] (4ms)
✓ should quality relative urls (5ms)
✓ should build card (6ms)
PASS test/open_graph/test.ts
✓ should build videos[] (7ms)
✓ should build images[] (3ms)
✓ should build audio[] (4ms)
✓ should quality relative urls (4ms)
✓ should build article[] (4ms)
PASS test/general/status-code.test.ts
✓ should throw if status code not 200 (19ms)
✓ should not throw if status code is 200 (4ms)
PASS test/basic/test.ts
✓ should handle content which is escaped badly (6ms)
✓ should detect title, description, keywords and canonical URL (5ms)
✓ should detect title, description, keywords and canonical URL even when they are in the body (3ms)
✓ should detect last dupe of title, description and keywords (4ms)
✓ should detect last dupe of title, description and keywords (3ms)
PASS test/general/options.test.ts
✓ should throw bad options error (10ms)
✓ should respect oembed (6ms)
PASS test/general/content-type.test.ts
✓ should throw bad content type error (22ms)
PASS test/general/url.test.ts
✓ should not throw when provided non-ascii url (3ms)
--------------------|----------|----------|----------|----------|-------------------|
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s |
--------------------|----------|----------|----------|----------|-------------------|
All files | 99.52 | 95.31 | 100 | 99.52 | |
index.ts | 99.5 | 95.31 | 100 | 99.49 | 376 |
schema.ts | 100 | 100 | 100 | 100 | |
unexpectedError.ts | 100 | 100 | 100 | 100 | |
--------------------|----------|----------|----------|----------|-------------------|
Handlebars: Access has been denied to resolve the property "statements" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Handlebars: Access has been denied to resolve the property "branches" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Handlebars: Access has been denied to resolve the property "functions" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Handlebars: Access has been denied to resolve the property "lines" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details
Test Suites: 1 skipped, 9 passed, 9 of 10 total
Tests: 1 skipped, 32 passed, 33 total
Snapshots: 0 total
Time: 1.637s, estimated 2s
Ran all test suites. |
🎉 This PR is included in version 6.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivation
There are four types of oEmbed: Photo, Video, Rich, and Link. And each of these has different fields that exist depending on the type. Therefore, it seems that the TypeScript type of oEmbed should be appropriately inferred according to the oEmbed type.
oEmbed's TypeScript types were implemented based on the following specification.