fix: marked and parse type overload with discrinated union options #3116

Yimiprod · 2023-12-01T12:25:18Z

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 · 2023-12-01T12:25:22Z

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 · 2023-12-01T12:52:16Z

src/Instance.ts


      // Show warning if an extension set async to true but the parse was called with async: false
-      if (this.defaults.async === true && origOpt.async === false) {
+      if (isAsyncOptions(this.defaults) && isSyncOptions(origOpt)) {
        if (!opt.silent) {
          console.warn('marked(): The async option was set to true by an extension. The async: false option sent to parse will be ignored.');


Question for maintainer: could this warning not be silent ?
Because as far as i know #3103 tried to fix the type to avoid unexepected Promises when a library change the default.async to true.

With this PR, the type returned by marked() and marked.parse() will have miss if a library change the default.async and user should have to be warned about it.

The silent option is for when they want things to fail instead of get warnings. If they want to see the error they can just set the silent option to false.

That is what #3103 tried to fix. It looks like this PR breaks that again. I think we still want correct types even if people need to add as string.

Well this PR doesn't break it pre see, it fixes the default typing based on default behavior (sending Promise if async is set to true).
So if the user want a fail instead of warning when a library modify the default, it's pretty ok ?

So you are suggesting throwing an error here instead of logging a massage? I think we could do that. This would need to be a breaking change.

Yimiprod · 2023-12-04T10:58:46Z

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 · 2023-12-04T15:38:03Z

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 · 2023-12-04T16:44:29Z

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 · 2023-12-04T16:45:38Z

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

UziTech · 2023-12-04T16:48:04Z

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 · 2023-12-04T16:49:38Z

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 · 2023-12-04T16:51:35Z

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 #3103 (comment) about two entry points.

vzakharov · 2024-02-08T06:33:33Z

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 · 2024-02-10T23:56:27Z

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 · 2024-02-12T10:34:49Z

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

Yimiprod · 2024-02-12T15:16:38Z

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

UziTech · 2024-02-12T15:46:03Z

src/Instance.ts

        if (!opt.silent) {
-          console.warn('marked(): The async option was set to true by an extension. The async: false option sent to parse will be ignored.');
+          throw new Error('marked(): The async option was set to true by an extension. The async: false option sent to parse will be ignored.');


Can you use the throwError function instead of throwing the error. Similar to other errors in this function. You will have the move the creation of the function above this if statement.

Sorry for the hiatus, just fixed this point, for the second point i'll try to figure out how to do it, since it need to modify typing in the .use() function to output the correct instance type.

UziTech · 2024-02-12T15:47:49Z

test/types/marked.ts

 const defaultMarked: string = marked(md);
-// as string can be used if no extensions set `async: true`
-const stringMarked: string = marked(md) as string;


Marked calls that don't specify an async option should be string | Promise<string> since they could be either and they won't throw an error.

since they could be either

Just curious, how’s that? If no async is specified, it defaults to false, right?

If no async is specified it should default to whatever async is set by extensions.

const syncMarked = new Marked({ async: false }); syncMarked.parse(md) // this will be a string const asyncMarked = new Marked({ async: true }); asyncMarked.parse(md) // this will be a Promise<string>

maybe there's a way to have typed returned instance ?
the default instance, without any modification, will be synch, right ?

const marked = new Marked(); marked.parse(md) // this will be a string if global config has not be mutated by an external library

Maybe. Typescript would have to run your application to see what the return type is.

Yes default without any extensions is sync

UziTech · 2024-03-10T15:20:26Z

src/Instance.ts

-        opt.async = true;
+      if (isAsyncOptions(this.defaults) && isSyncOptions(origOpt)) {
+        // Throw an error if an extension set async to true but the parse was called with async: false
+        return throwError(new Error('marked(): The async option was set to true by an extension. The async: false option sent to parse will be ignored.'));


The error message should be:

marked(): The async option was set to true by an extension. Remove the async: false option to continue.

UziTech · 2024-03-10T15:53:18Z

src/MarkedOptions.ts

+}
+
+export function isSyncOptions(options: MarkedOptions): options is MarkedSyncOptions {
+  return options.async === false;


Should this be false or undefined? Or maybe != true?

I pushed something like this:

return !isAsyncOptions(options);

It fail a lot of tests since it introduce a breaking change, i have to dive in to understand why.

…ith async at false BREAKING CHANGE: parser now throw an error when `defaultConfig` is modified by an extension to make the returned value a Promise and the we call parser with `async` option at false. It can be skipped with `silent` to true, making it still working for library that use it to modify the config globally but still calling each instance with the option `async` at false.

vercel bot deployed to Preview December 1, 2023 12:25 View deployment

Yimiprod force-pushed the type-overloads branch from f407a72 to f2b2219 Compare December 1, 2023 12:31

vercel bot deployed to Preview December 1, 2023 12:32 View deployment

Yimiprod force-pushed the type-overloads branch from f2b2219 to 1af8151 Compare December 1, 2023 12:32

vercel bot deployed to Preview December 1, 2023 12:33 View deployment

Yimiprod force-pushed the type-overloads branch from 1af8151 to 76c9a43 Compare December 1, 2023 12:35

vercel bot deployed to Preview December 1, 2023 12:36 View deployment

Yimiprod commented Dec 1, 2023

View reviewed changes

webstech mentioned this pull request Dec 4, 2023

build(deps): bump marked from 10.0.0 to 11.0.0 gitgitgadget/gitgitgadget#1493

Closed

dscho mentioned this pull request Dec 18, 2023

build(deps): bump marked from 10.0.0 to 11.0.1 gitgitgadget/gitgitgadget#1503

Closed

Yimiprod force-pushed the type-overloads branch from 76c9a43 to 1e602f2 Compare February 12, 2024 15:15

vercel bot deployed to Preview February 12, 2024 15:16 View deployment

UziTech requested changes Feb 12, 2024

View reviewed changes

vercel bot deployed to Preview March 9, 2024 08:17 View deployment

fix: marked and parse type overload with discrinated union options

784a9a9

Yimiprod force-pushed the type-overloads branch from ffdbeec to 451dad4 Compare March 9, 2024 08:19

vercel bot deployed to Preview March 9, 2024 08:19 View deployment

Yimiprod force-pushed the type-overloads branch from 451dad4 to 3e7320a Compare March 9, 2024 08:53

vercel bot deployed to Preview March 9, 2024 08:54 View deployment

UziTech requested changes Mar 10, 2024

View reviewed changes

Yimiprod force-pushed the type-overloads branch from 3e7320a to 66199ad Compare March 11, 2024 10:47

vercel bot deployed to Preview March 11, 2024 10:48 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: marked and parse type overload with discrinated union options #3116

fix: marked and parse type overload with discrinated union options #3116

Yimiprod commented Dec 1, 2023 •

edited

vercel bot commented Dec 1, 2023 •

edited

Yimiprod Dec 1, 2023 •

edited

UziTech Dec 1, 2023

UziTech Dec 1, 2023

Yimiprod Dec 4, 2023 •

edited

UziTech Dec 4, 2023

Yimiprod commented Dec 4, 2023 •

edited

UziTech commented Dec 4, 2023 •

edited

Yimiprod commented Dec 4, 2023

UziTech commented Dec 4, 2023

UziTech commented Dec 4, 2023

UziTech commented Dec 4, 2023

Yimiprod commented Dec 4, 2023 •

edited

vzakharov commented Feb 8, 2024 •

edited

UziTech commented Feb 10, 2024

Yimiprod commented Feb 12, 2024

Yimiprod commented Feb 12, 2024

UziTech Feb 12, 2024

Yimiprod Mar 9, 2024

UziTech Feb 12, 2024

vzakharov Feb 12, 2024

UziTech Feb 12, 2024

UziTech Feb 12, 2024

Yimiprod Feb 13, 2024 •

edited

UziTech Feb 13, 2024

UziTech Mar 10, 2024

UziTech Mar 10, 2024

Yimiprod Mar 11, 2024 •

edited

fix: marked and parse type overload with discrinated union options #3116

Are you sure you want to change the base?

fix: marked and parse type overload with discrinated union options #3116

Conversation

Yimiprod commented Dec 1, 2023 • edited

Description

Expectation

Result

Contributor

Committer

vercel bot commented Dec 1, 2023 • edited

Yimiprod Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yimiprod Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yimiprod commented Dec 4, 2023 • edited

UziTech commented Dec 4, 2023 • edited

Yimiprod commented Dec 4, 2023

UziTech commented Dec 4, 2023

UziTech commented Dec 4, 2023

UziTech commented Dec 4, 2023

Yimiprod commented Dec 4, 2023 • edited

vzakharov commented Feb 8, 2024 • edited

UziTech commented Feb 10, 2024

Yimiprod commented Feb 12, 2024

Yimiprod commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yimiprod Feb 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yimiprod Mar 11, 2024 • edited

Choose a reason for hiding this comment

Yimiprod commented Dec 1, 2023 •

edited

vercel bot commented Dec 1, 2023 •

edited

Yimiprod Dec 1, 2023 •

edited

Yimiprod Dec 4, 2023 •

edited

Yimiprod commented Dec 4, 2023 •

edited

UziTech commented Dec 4, 2023 •

edited

Yimiprod commented Dec 4, 2023 •

edited

vzakharov commented Feb 8, 2024 •

edited

Yimiprod Feb 13, 2024 •

edited

Yimiprod Mar 11, 2024 •

edited