fix: marked and parse type overload with discrinated union options

[Yimiprod]

Marked version: 11.0.0

Description

Fixes typescript expected error which force to use as string or as Promise or ts-expect-error comments by using disciminated union for options.

Added two type guards to detect which options object is used.

Expectation

• as string or as Promise<string> can be safely removed when using marked() and marked.parse() with respective config object
• No breaking change for library mutating default options

Result

No regression, no behavior change

Contributor

• [ ] Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
• [ ] no tests required for this PR.
• [ ] If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

• [ ] CI is green (no forced merge required).
• [ ] Squash and Merge PR following conventional commit guidelines.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marked-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 11, 2024 10:48am

[Yimiprod]

Has it as been said in #3103 there's no way to know if the default async has been changed, is it the responsibility of marked to know that the default behavior has been changed from an external library ?

The idea of this PR is to have a known return type based on the passed configuration which #3103 kinda broke by forcing to user to use as string even if we don't use a library that modify the default configuration.

With this PR:

(async() => {
  const md = '# foobar';
  const syncMarked: string = marked(md, { async: false }); // will work at compile time, but break at runtime if default has been changed
  const asyncMarked: string = marked(md, { async: true }); // will break at compile time since return type will be `Promise<String>`

  const syncMarkedParse: string = marked.parse(md, { async: false }); // will work at compile time, but break at runtime if default has been changed
  const asyncMarkedParse: string = marked.parse(md, { async: true }); // will break at compile time since return type will be `Promise<String>`

  const awaitedDefaultMarked: string = await marked(md); // will not break at compile time, will not break at runtime, even if default has been changed
  const awaitedDefaultMarkedParse: string = await marked.parse(md); // will not break at compile time, will not break at runtime, even if default has been changed
})();

Maybe when changing type to:

function marked(src: string, options: MarkedAsyncOptions): Promise<string>;
function marked(src: string, options: MarkedSyncOptions): string;
function marked(src: string, options?: MarkedOptions & { async?: undefined }): Promise<string> | string;
//or
function marked(src: string, options?: Exclude<MarkedOptions, 'async'>): Promise<string> | string;

But that mean introducing a breaking change here: https://github.com/markedjs/marked/blob/master/src/Instance.ts#L257

- if (isAsyncOptions(this.defaults) && isSyncOptions(origOpt)) {
+ if (isAsyncOptions(this.defaults) && typeof origOpt.async === 'undefined') { 

Which mean, fallback to default option, even overloaded by library, only if the developper does not have explicitly asked for an expected behavior.

[UziTech]

Before #3103 and if we merge this PR the type retuned be marked.parse could be wrong. If you are ok with not knowing the correct return type, just don't use typescript.

What is the difference from a user adding marked.parse(markdown, { async: false}) and marked.parse(markdown) as string? I think the second one is better since it is explicit what you are doing.

[Yimiprod]

The second one will break at runtime if a library change the default config and dev not knowing it will don't understand why since they force casted a type to the output.

[UziTech]

The first one will also break at runtime with this PR and before #3103

[UziTech]

So I think the only solution to not break at runtime if the developer doesn't know if they added an async extension is to leave it the way it is and await marked.parse

[UziTech]

If the developer does know that they didn't add an async extension than as string seems to be the cleanest solution to me.

[Yimiprod]

True that, that's why suggested adding a breaking change in my previous comment, making the behavior more explicit: If the developper explicitly pass async: true returning Promise or async: false returning string marked will behave as expected. Else if nothing is set, return Promise or string, based on default.

I don't know what impact it will have for library using marked, but since it's a breaking change, this will change the major version of marked, this way preventing update for user prefixing their package.json version with at least ^ ?

Edit: i'm eager to create a new PR for that, including documentation update, since it was also a point in #3103 by this comment https://github.com/markedjs/marked/pull/3103#issuecomment-1831388249 about two entry points.

[vzakharov]

Can you please merge this? Having .parse return | Promise when it definitely isn’t one is such a bugger.

For the time being, if anyone’s as bothered as I am, here’s a helper method you can use:

import { MarkedOptions, marked } from "marked";

export function parseMarkdown(markdown: string, options: Omit<MarkedOptions, 'async'> = {}) {
  const parsed = marked.parse(markdown, options);
  if ( typeof parsed === 'string' ) return parsed;
  throw new Error(`Expected marked to return a string, but it returned ${parsed}`);
};

[UziTech]

If this was complete we could merge it in the next major version.

A couple of things that need to be fixed:

• throw an error instead of warn when async: false is used with an async extension.
• test new Marked().parse(md, { async: true/false }) returns the correct type.
• fix merge conflicts

[Yimiprod]

I'll try to find some time to fix it as soon as possible 👍

[Yimiprod]

@UziTech added a new commit that replace the warning with a throw, and adding tests reflecting this changes. The commit is flagged as breaking change (but i can remove it if you want).