Export slugify function

[jackyef]

Hey, thanks for the helpful library!

I recently encountered an edge case related to the heading id. In short, our application was calling something like analytics.push() and for some reasons the invocation was failing in a page that renders the <Markdown /> component.

After a closer inspection, apparently it was caused by a ### Analytics in our markdown content. The <h3> element was assigned an id="analytics" and the browser assigned this to the global scope (e.g.: window), causing analytics.push() to no longer be a function. This is due to the related 3rd party library doing initialization as follows:

var analytics = window.analytics || [];

analytics.push();

The solution in this PR is to prefix the id with a -, making it an invalid JS variable name so it can't pollute the global scope. The browser behavior of automatically scrolling to the corresponding landmark still works as expected (e.g.: visiting /page#-section-heading will still automatically scroll the page to element with id="-section-heading".

The solution in this PR is exporting the slugify function so users can easily augment it to avoid the issue.

[changeset-bot]

🦋 Changeset detected

Latest commit: ebccbc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
markdown-to-jsx	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[quantizor]

Makes sense to me :-)

[quantizor]

@jackyef could you add a changeset please with the explanation of what it's fixing and why?

[jackyef]

@quantizor I added a changeset file, not sure if the wordings fit the convention of the project, feel free to edit it as you please!

[quantizor]

@quantizor I added a changeset file, not sure if the wordings fit the convention of the project, feel free to edit it as you please!

Looks perfect 👍🏼

[quantizor]

Thinking about this further, it is kind of a breaking change. I'm wondering if we should make this the default behavior in v8 and for v7 ship the original slugify function so it can be composed to do the desired behavior?

So basically we do this:

// note the added `export` statement
export function slugify(str: string) {
  return str
    .replace(/[ÀÁÂÃÄÅàáâãäåæÆ]/g, 'a')
    .replace(/[çÇ]/g, 'c')
    .replace(/[ðÐ]/g, 'd')
    .replace(/[ÈÉÊËéèêë]/g, 'e')
    .replace(/[ÏïÎîÍíÌì]/g, 'i')
    .replace(/[Ññ]/g, 'n')
    .replace(/[øØœŒÕõÔôÓóÒò]/g, 'o')
    .replace(/[ÜüÛûÚúÙù]/g, 'u')
    .replace(/[ŸÿÝý]/g, 'y')
    .replace(/[^a-z0-9- ]/gi, '')
    .replace(/ /gi, '-')
    .toLowerCase()
}

There already is an options.slugify customization point so to fix your issue you'd do something like this:

import { slugify } from 'markdown-to-jsx';

options={{
  slugify: str => {
    let result = slugify(str)

    return result ? '-' + str : result;
  }
}}

That fixes things for v7 immediately for your use and then in v8 we will add that functionality as standard. Sound good?

[jackyef]

Actually, now that I think about it. I am thinking about this all wrong. Prefixing the id with - doesn't make it an invalid variable name. window['-analytics'] is totally valid. The logic behind my initial proposed solution isn't sound, we shouldn't ship it in v8 either, apologies for the back and forth😅

I agree. The more simple solution of exporting slugify out of the library is nicer. I'll update the PR accordingly.

[quantizor]

Actually, now that I think about it. I am thinking about this all wrong. Prefixing the id with - doesn't make it an invalid variable name. window['-analytics'] is totally valid. The logic behind my initial proposed solution isn't sound, we shouldn't ship it in v8 either, apologies for the back and forth😅

I agree. The more simple solution of exporting slugify out of the library is nicer. I'll update the PR accordingly.

I liked your idea from the PoV that it's less likely to conflict with global variables. Can bikeshed in v8 👍