Add onComplete callback for completing processing in MarkdownHooks

[BlackRam-oss]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and discussions and couldn’t find anything or linked relevant results below
• I made sure the docs are up to date
• I included tests (or that’s not needed)

Description of changes

Add onComplete callback for completing processing in MarkdownHooks

close #912

[remcohaszing]

I’m not really for or against this change. I see why it’s useful, but I also wonder if we should add such features. react-markdown is a relatively simple, high-level library. All the pieces are available in the unified ecosystem to build a more specialized integration if the user needs it.

[JounQin]

Please open a new issue before PR next time.

[BlackRam-oss]

Please open a new issue before PR next time.

Hello @JounQin

I created an issue, but I'm afraid I didn't manage to link it #912

Anyway, yes, next time I start editing I'll wait for approval.

[JounQin]

I created an issue, but I'm afraid I didn't manage to link it #912

Anyway, yes, next time I start editing I'll wait for approval.

Let's convert this PR into draft and discuss with other maintainers there then.

By the way, the CI is still broken.

[BlackRam-oss]

@JounQin

Unfortunately I was not able to understand what the CI error is.

All tests went well but it gives the following error:
ERROR: Coverage for branches (99.55%) does not meet global threshold (100%)

[JounQin]

All tests went well but it gives the following error:

ERROR: Coverage for branches (99.55%) does not meet global threshold (100%)

You didn't provide a new test case for options.onComplete at all.

[ChristianMurphy]

I won't block this, but am not sure I'd recommend it either.

I don't think JS is needed to make a typing animation, CSS can likely work well enough, see https://github.com/orgs/remarkjs/discussions/1321 and https://github.com/orgs/remarkjs/discussions/1310

onComplete callback

names like onComplete and onDone imply that they refer to a promise, but here it doesn't

ERROR: Coverage for branches (99.55%) does not meet global threshold (100%)

The tests measure code coverage https://en.wikipedia.org/wiki/Code_coverage
Branch coverage, the measure of conditions in the code that are run by tests, is missing some cases from tests, so is failing.

[wooorm]

If this was done, I: a) worry about the name; b) think that probably no parameters should be passed at all? c) there is no error handling; d) there should probably be a result handling?

But importantly, I worry what this is for. Have you actually tried to use this? It looks like it could be abused to change the DOM that we control. And I think this sets expectations to users that they can modify file/tree/error but it wouldn’t actually do what they wanted.

[BlackRam-oss]

@ChristianMurphy

I don't think JS is needed to make a typing animation, CSS can likely work well enough, see https://github.com/orgs/remarkjs/discussions/1321 and https://github.com/orgs/remarkjs/discussions/1310

I switched from using CSS to using motion with https://motion.dev/docs/react-transitions#staggerchildren (which I believe still uses mostly CSS) because I needed to intercept some animation events (for example the completion of the animation of the entire text and certain elements of the text such as punctuation) and give the user the possibility to have total control of the animations in an easy way.
There is probably the possibility to achieve a similar result with CSS (with a lot of work), but I don't think I would have been able to give the possibility to the user of my library to be able to modify it in an easy way.

@wooorm

But importantly, I worry what this is for

In any case I imagine that also in other cases those who are using MarkdownHooks might need to know if the user has viewed the text or just the loading.

For example in a very common case that is the management of notifications (with a very long text), in particular if it has been read or not. Those who are using MarkdownHooks to display the text of messages might need to notify the bakend that the message has been read by the user only after MarkdownHooks has finished loading.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (fda7fa5) to head (e2c1dcf).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #913   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines         1743      1770   +27     
  Branches       123       125    +2     
=========================================
+ Hits          1743      1770   +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[remcohaszing]

Note that in the callback, it hasn’t been rendered. Only the React state was just updated.

There are ways to actually know when it’s done rendering. For example,

import { Root } from 'hast'
import { createContext, ReactNode, Ref, use, useState } from 'react'

const RenderedContext = createContext<Ref<HTMLSpanElement>>()

function Span({ 'data-rendered': rendered, ...props }): ReactNode {
  return <span {...props} ref={rendered ? use(RenderedContext) : null} />
}

const components = {
  span: Span
}

function rehypeRenderedIndicator() {
  return (ast: Root) => {
    ast.children.push({
      type: 'element',
      tagName: 'span',
      properties: {
        'data-rendered': 'true'
      },
      children: [],
    })
  }
}

const rehypePlugins = [rehypeRenderedIndicator]

function App(): ReactNode {
  const [rendered, setRendered] = useState<HTMLSpanElement>()

  return (
    <RenderedContext value={setRendered}>
      <MarkdownHooks rehypePlugins={rehypePlugins}>{''}</MarkdownHooks>
    </RenderedContext>
  )
}

[JounQin]

There are ways to actually know when it’s done rendering. For example

Oh my eyes.

[wooorm]

For example in a very common case that is the management of notifications (with a very long text), in particular if it has been read or not. Those who are using MarkdownHooks to display the text of messages might need to notify the bakend that the message has been read by the user only after MarkdownHooks has finished loading.

This proposed feature is not a good solution for that use case

[BlackRam-oss]

This proposed feature is not a good solution for that use case

@wooorm, Why? Is the problem where the onComplete function is executed?

---

The fact of not giving the possibility to access the file, hast and error, and the fact that the name onComplete could be not apriopiated, I can understand and honestly for me it is indifferent.

However I did not understand if there are cons on the fact of adding a function to notify the developer of the completion.