Fix `join` order - Githubissues

vvenv commented 3 years ago

Initial checklist

[x] I read the support docs
[x] I read the contributing guide
[x] I agree to follow the code of conduct
[x] I searched issues and couldn’t find anything (or linked relevant results below)
[x] If applicable, I’ve added docs and tests

Description of changes

join from options should have a higher priority. It's useful for handling list/listItem spread logic.

wooorm commented 3 years ago

join from options should have a higher priority.

Why?

It's useful for handling list/listItem spread logic.

Yes, but it already supports that?

the test join function your provide returns zero lines between anything, which will break markdown. Can you provide the actual use case you have?

wooorm commented 3 years ago

It's useful for handling list/listItem spread logic.

You can change the spread field on nodes for that?

vvenv commented 3 years ago

join from options should have a higher priority.

Why?

It's useful for handling list/listItem spread logic.

Yes, but it already supports that?

the test join function your provide returns zero lines between anything, which will break markdown. Can you provide the actual use case you have?

The test case is the actual use case (If I add list/listitem conditions to the join function). In the case, I want to remove all unneccesary newLine to remove unneccesary <p>, just like what CKEDITOR5 is doing.

Here is the real case, a markdown WYSIWYG editor based on CKEDITOR5:

https://g13mo.codesandbox.io/

The extra new lines make the test html2markdown(markdown2html(value)) === value broken.

vvenv commented 3 years ago

It's useful for handling list/listItem spread logic.

You can change the spread field on nodes for that?

Yes, I can, with a custom plugin.

But I really think it's more common to increase the priority of custom join.

wooorm commented 3 years ago

The code you linked does not include mdast-util-to-markdown — what am I looking at?

The extra new lines make the test html2markdown(markdown2html(value)) === value broken.

Sounds like html2markdown(markdown2html(value)) is broken?

Yes, I can, with a custom plugin.

This is my suggestion

The test case is the actual use case (If I add list/listitem conditions to the join function). In the case, I want to remove all unneccesary newLine to remove unneccesary <p>, just like what CKEDITOR5 is doing.

Please show that actual code, because right now you’re breaking all of markdown.

How many join functions do you have? If you return undefined from a join, the next one already kicks in.

Assuming you do not pass tightDefinitions, then there are no join functions, meaning the order is exactly as how you pass them. You can pass them inverted yourself

github-actions[bot] commented 3 years ago

Hi! Thanks for taking the time to contribute! This has been marked by a maintainer as needing more info. It’s not clear yet whether this is an issue. Here are a couple tips:

Spend time framing the pull request! The more time you put into it, the more we will
Often, maintainers respond with why for several back and forths; rubber duck debugging might help avoid that
Folks posting issues sometimes fall for xy problems: asking for a certain solution instead of raising the root problem

Thanks, — bb

vvenv commented 3 years ago

The code you linked does not include mdast-util-to-markdown — what am I looking at?

The extra new lines make the test html2markdown(markdown2html(value)) === value broken.

Sounds like html2markdown(markdown2html(value)) is broken?

Please show that actual code, because right now you’re breaking all of markdown.

How many join functions do you have? If you return undefined from a join, the next one already kicks in.

Assuming you do not pass tightDefinitions, then there are no join functions, meaning the order is exactly as how you pass them. You can pass them inverted yourself

The source code of the code I linked: https://github.com/crossjs/ckeditor5/blob/master/packages/ckeditor5-markdown-gfm/src/html2markdown.js#L14-L36
html2markdown(markdown2html(value)) should equal value, but NOT.
There is always a default join return 1 or 0 always in the case, that make custom join never hit.

After all, the change really breaking all of markdown.

Maybe a custom plugin is the best solution. Maybe I should do more try and test.

Thanks for your patience.

wooorm commented 3 years ago

Ah, nice catch, I get it now!

wooorm commented 3 years ago

html2markdown(markdown2html(value)) should equal value, but NOT.

Are you using mdast-util-to-hast and hast-util-to-mdast for this? Or something else? Sounds like that is a separate bug?

github-actions[bot] commented 3 years ago

Hi! This is accepted and can go somewhere!

Team: please review this PR and use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this.

vvenv commented 3 years ago

html2markdown(markdown2html(value)) should equal value, but NOT.

Are you using mdast-util-to-hast and hast-util-to-mdast for this? Or something else? Sounds like that is a separate bug?

Yes, for some plugins.

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

Thanks, released! https://github.com/syntax-tree/mdast-util-to-markdown/releases/tag/1.0.1

syntax-tree / mdast-util-to-markdown

Fix `join` order #32

Initial checklist

Description of changes