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.

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 null | undefined to parse results. Fixes #937 #944

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

danielnixon
Copy link
Contributor

Fixes #937.

This PR might be too much of a swing in the opposite direction. The reason I've included null and undefined in all props is because it's difficult to trace exactly which props are always assigned a value versus which might be null versus which might be null or undefined.

I've got a branch locally where I'm slowly annotating everything with types, but that is slow going. In the meantime, erring this way is arguably more appropriate.

@danielnixon
Copy link
Contributor Author

An alternative option here (slightly more invasive) could be to coalesce everything in the object returned from parse with either null or undefined, then only include one or the other in the type definition. Something like:

return {
      title: this._articleTitle ?? undefined,
      byline: metadata.byline || this._articleByline ?? undefined,
      dir: this._articleDir ?? undefined,
      lang: this._articleLang ?? undefined,
      content: this._serializer(articleContent) ?? undefined,
      textContent: textContent  ?? undefined,
      length: textContent.length  ?? undefined,
      excerpt: metadata.excerpt  ?? undefined,
      siteName: metadata.siteName || this._articleSiteName  ?? undefined,
      publishedTime: metadata.publishedTime  ?? undefined,
    };

(or null instead of undefined if that is the preference).

Copy link

@cmkm cmkm left a comment

Choose a reason for hiding this comment

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

Thank you! If this doesn't work for yourself or other Typescript users in the future we can revisit this with your alternate solution.

@cmkm cmkm merged commit 8c0ff87 into mozilla:main Jan 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript type returned by parse is misleading
2 participants