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

Fix duplicate heading id #1401

Merged
merged 3 commits into from Dec 31, 2018

Conversation

@styfle
Copy link
Member

commented Dec 20, 2018

Marked version: 0.5.2

Markdown flavor: All

Description

This fixes several bugs involving headerIds: true.

This adds a new export marked.Slugger.

I added several more tests to cover duplicates as well as ampersands.

Contributor

  • Test(s) exist to ensure functionality and minimize regression

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR
test/unit/marked-spec.js Outdated Show resolved Hide resolved
@UziTech

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

we also need to test for Flet/github-slugger#20

    expect(slugger.slug('test 1')).toBe('test-1');
    expect(slugger.slug('test')).toBe('test');
    expect(slugger.slug('test')).toBe('test-2');

and

  expect(slugger.slug('foo'), 'foo')
  expect(slugger.slug('foo'), 'foo-1')
  expect(slugger.slug('foo 1'), 'foo-1-1')
  expect(slugger.slug('foo-1'), 'foo-1-2')
  expect(slugger.slug('foo'), 'foo-2')
@styfle

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

That's an interesting problem you found. I fixed this PR, thanks 👍

@UziTech
Copy link
Member

left a comment

Looks great 👍

@styfle styfle requested a review from davisjam Dec 22, 2018

@styfle

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

@davisjam @joshbruce After this PR lands, I can do a new release. Can you take a look?

@joshbruce joshbruce merged commit dfbc9a1 into markedjs:master Dec 31, 2018

3 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (markedjsbot) No known issues
Details
@joshbruce

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Just to make sure I'm up to speed on the header conversation and whatnot. We're adding slugger to fix a defect, which is header duplication. We are still experiencing issues with unicode characters like Japanese and other glyph languages. And we are still progressing toward dropping header ID generation in the future. Yeah?

@styfle styfle deleted the styfle:slugger2 branch Dec 31, 2018

@styfle

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2018

@joshbruce This PR fixes non-latin char headings as well as duplicate IDs.

In the future we could drop support and rely on github-slugger. That PR #1354 (which was removing slugger from marked) was rejected. So I made this PR.

@tmorehouse

This comment has been minimized.

Copy link

commented Jan 1, 2019

@styfle @joshbruce @UziTech

We're receiving errors for this latest update (^0.5.2 -> ^0.6.0):

8:57:27 PM: WARNING in ./src/components/table/README.md
8:57:27 PM: Module build failed (from ./node_modules/markdown-loader/index.js):
8:57:27 PM: TypeError: Cannot read property 'slug' of undefined
8:57:27 PM: Please report this to https://github.com/markedjs/marked.
8:57:27 PM:     at Renderer.heading (/opt/build/repo/node_modules/marked/lib/marked.js:962:17)
8:57:27 PM:     at Parser.tok (/opt/build/repo/node_modules/markdown-loader/node_modules/marked/lib/marked.js:1179:28)
8:57:27 PM:     at Parser.parse (/opt/build/repo/node_modules/markdown-loader/node_modules/marked/lib/marked.js:1130:17)
8:57:27 PM:     at Function.Parser.parse (/opt/build/repo/node_modules/markdown-loader/node_modules/marked/lib/marked.js:1112:17)
8:57:27 PM:     at marked (/opt/build/repo/node_modules/markdown-loader/node_modules/marked/lib/marked.js:1543:19)
8:57:27 PM:     at Object.module.exports (/opt/build/repo/node_modules/markdown-loader/index.js:14:12)
@tmorehouse tmorehouse referenced this pull request Jan 1, 2019
0 of 1 task complete
@styfle

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

Are you overwriting the renderer methods?
See the release notes for breaking changes.

https://github.com/markedjs/marked/releases/tag/v0.6.0

If so, try passing new Slugger()
If not, please file a new issue with steps to reproduce.

Thanks!

@tmorehouse

This comment has been minimized.

Copy link

commented Jan 1, 2019

We are adding a wrapper on the table renderer so that must be it. Will try restoring the slugger handler.

@tmorehouse

This comment has been minimized.

Copy link

commented Jan 1, 2019

It appears that markdown-loader is using 0.5.x, which is causing our issue (as we are passing it a new renderer instance with code and table overrides), but our renderer is v0.6.0, which has a different calling signature (the slugger argument), compared to v0.5.x

@541xxx 541xxx referenced this pull request Jun 5, 2019
3 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.