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

Updating remark, unified etc #342

Closed
wants to merge 18 commits into from
Closed

Updating remark, unified etc #342

wants to merge 18 commits into from

Conversation

vhf
Copy link
Contributor

@vhf vhf commented Jul 20, 2019

This branch is tracking @arobase-che 's progress on upgrading remark, unified, etc

Set and Promise are now global by default.
This commit is about updating how the id of the footnote is retrived.
Since, mdast-util-to-hast/issues/14, footnote link backref are wrapped
inside the paragraph which is wrapped inside the `li` tag.

Using unist-util-visit-parents instead of unist-util-visit to retrive
the `li` tag.
**This commit only convert new identifiers to string.**
Identifiers used to be strings but in the previous version of this plugin,
identifiers were converted to numbers.

Now the type is preserved.

Adapt the plugin to the commit of mdast-util-to-hast#fd38c454
See the issue: syntax-tree/mdast-util-to-hast#32

It updates the order of footnotes to a more coherent one.
But identifiers no longer allowed to be number.
@artragis
Copy link
Member

Can I help here for somthing?

@arobase-che
Copy link
Contributor

arobase-che commented Oct 3, 2019

Hi, I'm still working on it.

I got a weird case.

Since : remarkjs/remark#424

Spaces in inline code are preserved. So a test case doesn't pass.

The test is :

New line: `\
`

The old result was « nothing ». Since the inline code became empty it was deleted.
Now, the backslash is escaped and the new line remains inside the inline code.

I don't understand the new behavior. I think it's not a useful test case and we should just ignore it. I have plenty of test case to study.

Can I just skip it and update the snapshot ? I will leave a issue on remark. GFM and CommonMark replace the new line by a space. Nothing interesting since the chose of remark seems legit.

@vhf
Copy link
Contributor Author

vhf commented Oct 3, 2019

I'm not sure I understand what the issue is, to me remark seems to default to the correct behavior. Apparently you are seeing something different that is not consistent with gfm: true?

Here's how the case you present should be rendered IMO:

New line: `\
`

=>

<p>New line: <code>\ </code></p>

Oct-03-2019 22-17-51

Basic example showing this behavior: https://github.com/vhf/remark-playground . Using .use(markdown, {gfm: true}) instead of https://github.com/vhf/remark-playground/blob/6f200e3a195f03453c5ffbf375619378430b0ba1/index.js#L9 doesn't change anything.

@arobase-che
Copy link
Contributor

arobase-che commented Oct 3, 2019

I'm not sure I understand what the issue is, to me remark seems to default to the correct behavior. Apparently you are seeing something different that is not consistent with gfm: true?

Yeap ! Here an example : https://github.com/arobase-che/remark-playground/tree/parsed

I don't know why your version works but mine is problematic with rebber.
https://github.com/arobase-che/remark-playground/tree/rebber

Here's how the case you present should be rendered IMO:

New line: `\
`

=>

<p>New line: <code>\ </code></p>

Oct-03-2019 22-17-51

Basic example showing this behavior: https://github.com/vhf/remark-playground . Using .use(markdown, {gfm: true}) instead of https://github.com/vhf/remark-playground/blob/6f200e3a195f03453c5ffbf375619378430b0ba1/index.js#L9 doesn't change anything.

I totally agree with you ^^

PS: Sorry about my English ._.

@arobase-che
Copy link
Contributor

arobase-che commented Oct 4, 2019

@vhf
Copy link
Contributor Author

vhf commented Oct 6, 2019

I don't know why your version works but mine is problematic with rebber.
https://github.com/arobase-che/remark-playground/tree/rebber

Here's my example again:

const inspect = require('util').inspect
const vfile = require('to-vfile')
const report = require('vfile-reporter')
const unified = require('unified')
const markdown = require('remark-parse')
const remark2rehype = require('remark-rehype')
const html = require('rehype-stringify')

unified()
  .use(markdown, {gfm: true})
  .use(() => (vfile) => { console.warn(inspect(vfile, false, 9999, true)) })
  .use(remark2rehype)
  .use(() => (vfile) => { console.warn(inspect(vfile, false, 9999, true)) })
  .use(html)
  .process(vfile.readSync('example.md'), function(err, file) {
    console.error(report(err || file))
    console.log(String(file))
  })

We see that remark-parse keeps \n:

        {
          type: 'inlineCode',
          value: '\\\n',

and then remark-rehype removes it:

        {
          type: 'element',
          tagName: 'code',
          properties: {},
          children: [ { type: 'text', value: '\\ ' } ],

I'll check all the places where collapse-white-space is used in mdast-util-to-hast and apply the same to the corresponding rebber/rebber-plugins code. :)

Btw, I'm suppose to edit the fixtures by hand ?

That's probably the safest thing to do, yes. Are there a lot of HTML changes? Will the HTML changes cause issues on zds?

@arobase-che
Copy link
Contributor

Ok :)

Btw, I'm suppose to edit the fixtures by hand ?

That's probably the safest thing to do, yes. Are there a lot of HTML changes? Will the HTML changes cause issues on zds?

Enough to be upsetting :(
Usually, it's a correction of remark. Half of them are about footnotes (paragraphs and numbers) or consecutives quotes that won't merge now.

Example:

> Here is one

> Here is another and not the same

@vhf
Copy link
Contributor Author

vhf commented Oct 21, 2019

Are there a lot of HTML changes? Will the HTML changes cause issues on zds?

Enough to be upsetting :(

Then please make sure people agree with these changes before we land this PR :)

@artragis
Copy link
Member

FOr quotes it clearly is what we want.

@vhf vhf closed this Jan 30, 2020
@vhf vhf deleted the update branch February 3, 2020 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants