syntax-tree / mdast-util-to-markdown

mdast utility to serialize markdown
http://unifiedjs.com
MIT License
100 stars 20 forks source link

Fix user handlers erases extension handlers #40

Closed jordimarimon closed 3 years ago

jordimarimon commented 3 years ago

Initial checklist

Description of changes

This fixes the case where a user defines handlers and extensions that also define more handlers.

User handlers and extension handlers will be merged together and only extension handlers with the same type name as user handlers will be overriden.

I haven't added any unit test because there is no public API to read the options.

Maybe in the future we could add a way to read the options so we can test that the merge is done correctly?

For more infornmation: GH-39

jordimarimon commented 3 years ago

It could be improved if we check if any user handler is overriding an extension handler and output a warning in the console so user knows that she/he is overriding an extension?

wooorm commented 3 years ago

I just made a test locally:

  t.equal(
    to(
      {
        type: 'root',
        children: [
          {
            type: 'paragraph',
            children: [
              {type: 'text', value: 'a'},
              {type: 'break'},
              {type: 'text', value: 'b'}
            ]
          }
        ]
      },
      {
        extensions: [
          {
            handlers: {
              text: (node) => node.value.toUpperCase(),
              break: () => '+'
            }
          }
        ],
        handlers: {
          break: () => '-'
        }
      }
    ),
    'A-B\n',
    'should support options in extensions'
  )

However, this already seems to work? I’m getting 'A-B\n'.

wooorm commented 3 years ago

What handler are you seeing is being removed? literal is not a node type, so it can’t be handled, maybe that might indicate why your earlier problem didn’t work?

jordimarimon commented 3 years ago

You're totally right, my fault. You're already doing it correctly the merge with Object.assign.

I have tested it now in the company project where I am using the library and it works totally fine.

I don't know why yesterday wasn't working. Yesterday I was getting the error in the container-phrasing.js in line 65 that slice wasn't a function.

At that moment I thought that I was erasing the extension handlers but that's totally not true because Object.assign does the same as the spread operator.

Maybe there was something wrong in my repository.

I'm using it like this:

const markdown = toMarkdown(tree, {
    extensions: [gfmToMarkdown()],
    handlers: {
       // Custom handler for adding text without escaping
       literal: (node, _parent, _context) => node.value || '',
    },
 });

And is working perfectly!

Although literal is not a node type, I can define my own node types if I provide a handler for them?

You can close this PR and the issue GH-39, both of them are wrong.

Sorry for the inconvenience (I don't know what I was thinking about Object.assign).

github-actions[bot] commented 3 years ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

wooorm commented 3 years ago

Ah, well glad it’s not a bug.

Although literal is not a node type, I can define my own node types if I provide a handler for them?

Yep, you can! Perhaps a more descriptive name might be better though, literal is an abstract node type for any node with a value, so that definition might be confusing?


The slice is not a function error might be because a non-string is returned by a handler, so node.value || '' should solve that

All the best!