quantizor / markdown-to-jsx

🏭 The most lightweight, customizable React markdown component.
https://markdown-to-jsx.quantizor.dev/
MIT License
1.96k stars 169 forks source link

Export `slugify` function #566

Closed jackyef closed 5 months ago

jackyef commented 5 months ago

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[bot] commented 5 months ago

🦋 Changeset detected

Latest commit: ebccbc112cf8e48a6c4e0995d7aee6ced8aad297

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 commented 5 months ago

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

jackyef commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 👍