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

effect.check() modify events when construct is for document and has resolver #86

Closed
4 tasks done
wataru-chocola opened this issue Aug 27, 2021 · 9 comments
Closed
4 tasks done
Labels
💪 phase/solved Post is done

Comments

@wataru-chocola
Copy link

Initial checklist

Affected packages and versions

micromark

Link to runnable example

https://github.com/wataru-chocola/report-micromark-20210827

Steps to reproduce

Run my PoC.

$ git clone https://github.com/wataru-chocola/report-micromark-20210827
$ cd report-micromark-20210827
$ npm install
$ npx node index.js

Expected behavior

document constructs are invoked twice in micromark/lib/initialize/document.js :

  1. from checkNewContainers state

    return effects.check(
      containerConstruct,
      thereIsANewContainer,
      thereIsNoNewContainer
    )(code)
  2. from documentContinued

    return effects.attempt(
      containerConstruct,
      containerContinue,
      flowStart
    )(code)

And I expect the first invocation effect.check(...) doesn't make any modifications on events.

 * @property {Attempt} check
 *   Attempt, then revert.

Actual behavior

effect.check() does modify events if construct is for document and has resolver.

My construct in PoC code dumps context.events at the start.
On 1st run (from effects.check), we see the correct events which are generated by previous tokenization.

+ initialize tokenizer (runCount: 1)
+ previous events
[ 'enter', 'chunkFlow', 'term\n' ]
[ 'exit', 'chunkFlow', 'term\n' ]
+ run resolverTo

But on 2nd run (from effects.attempt), events are modified by resolver in the previous check execution.

+ initialize tokenizer (runCount: 2)
+ previous events
[ 'enter', 'defListTerm', 'term\n' ]
[ 'enter', 'chunkFlow', 'term\n' ]

Runtime

No response

Package manager

No response

OS

No response

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 27, 2021
@wooorm
Copy link
Member

wooorm commented Aug 27, 2021

Maybe I can help better if you share what you want to do?
Perhaps it’s possible to not use containers? Containers are quite complex.
And indeed, resolve changes the events.

@wataru-chocola
Copy link
Author

@wooorm Thank you for your reply.

I've been trying to implement a micromark extension for definition list in this spec.
For example, following markdown

Apple
:   Pomaceous fruit of plants of the genus Malus in 
    the family Rosaceae.

Orange
:   The fruit of an evergreen tree of the genus Citrus.

would makes the following HTML.

<dl>
<dt>Apple</dt>
<dd>Pomaceous fruit of plants of the genus Malus in 
the family Rosaceae.</dd>

<dt>Orange</dt>
<dd>The fruit of an evergreen tree of the genus Citrus.</dd>
</dl>

I read setext-underline.js and list.js and imitated them because their syntaxes look like one which I wanted to handle.
Is there any way to implement this syntax extension without containers?

@wooorm
Copy link
Member

wooorm commented Aug 28, 2021

If you really want to do this, I think you might be better of looking at footnote definitions for the “container” part, those colons, see: https://github.com/micromark/micromark-extension-footnote/blob/561f958a9bf33a1f3f0d2f61b370199dbff2610b/dev/lib/syntax.js#L45-L49.

But, honestly, I think this is a bad designed syntax extension, that doesn’t match how markdown works otherwise.

Better solutions in my opinion are either directives (Arbitrary extension mechanism in § Extending markdown):

:::definition[Orange]
The fruit of an evergreen tree of the genus Citrus.
:::

:::definition[Apple]
*   Pomaceous fruit of plants of the genus Malus in 
    the family Rosaceae.

*   An American computer company.
:::

:::definition[Term 1, Term 2]
Definition a
:::

Or (ab)using lists (Using and abusing markdown to add new meaning in § Extending markdown):

*   **Orange**

    The fruit of an evergreen tree of the genus Citrus.

*   **Apple**

    *   Pomaceous fruit of plants of the genus Malus in 
        the family Rosaceae.

    *   An American computer company.

*   **Term 1**
    **Term 2**

    Definition a

@wataru-chocola
Copy link
Author

@wooorm Thank you, I'll take a look at micromark-extension-footnote.

And indeed, resolve changes the events.

Well, the behavior about effect.check / effect.attempt / resolveTo seems a bit inconsistent for me.

  • effect.attempt (onsuccessfulconstruct) calls addResult which invokes resolveTo to modify events. This is straightfoward.
  • effect.check (onsuccessfulcheck) doesn't do, but if its argument construct calls effect.attempt inside it, it would run resolveTo and modify events consequently.

If this behavior is not a problem, please close this issue.

And thank you for your help!

@wataru-chocola
Copy link
Author

Just FYI.

Thanks to your kind help, I've finally published my extension package.

Regards

@wooorm
Copy link
Member

wooorm commented Sep 8, 2021

Awesome, glad it worked!

I looked through the readmes and here are some suggestions, If you’re interested:

  • The remark-definition-list usage example uses ./index, but should probably import the package name?
  • For remark plugins, I recommend using a default export for the plugin. This means they can be used by the engine (remark-cli), such as with remark input.md --use remark-definition-list --output output.md (only with a default can the engine/cli “find” the plugin).
    All other plugins also use a default export, so it’ll help users because there is a single API
    You can export the same value as both a named export and a default export btw, so it can be a minor release!
  • You can add the plugin to the List of plugins in remark’s readme https://github.com/remarkjs/remark/blob/main/doc/plugins.md#list-of-plugins
  • Same for the extension here, maybe a new “Community extensions” heading, with this extension, under the current list of extensions? https://github.com/micromark/micromark#list-of-extensions
  • There’s a typo in the micromark extension readme: Definitio instead of Definition. Also develepment -> development!

👋

@wooorm
Copy link
Member

wooorm commented Sep 8, 2021

Going to close this, as this particular thing is intended. Maybe some docs somewhere might be good, and I’d appreciate a PR for that if you have a suggestion!

@wooorm wooorm added the 💪 phase/solved Post is done label Sep 8, 2021
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Sep 8, 2021
@wataru-chocola
Copy link
Author

@wooorm

I published new version including your suggestions, bug fix, some improvement.
Can I ask you to add my plugin and extension to remark / micromark readme?

wooorm added a commit that referenced this issue Sep 12, 2021
@wooorm
Copy link
Member

wooorm commented Sep 12, 2021

Done!

@wooorm wooorm closed this as completed Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

No branches or pull requests

2 participants