close
Skip to content

Add null | undefined to parse results. Fixes #937#944

Merged
cmkm merged 1 commit intomozilla:mainfrom
danielnixon:937
Jan 9, 2025
Merged

Add null | undefined to parse results. Fixes #937#944
cmkm merged 1 commit intomozilla:mainfrom
danielnixon:937

Conversation

@danielnixon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

@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
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