search

alphagov / govuk-design-system

One place for service teams to find styles, components and patterns for designing government services.
https://www.gov.uk/design-system
MIT License
509 stars 231 forks source link

Use the same version of `marked` in our build and our plugins #2767

Closed domoscargin closed 1 year ago

domoscargin commented 1 year ago

What

Sync marked version/source across:

@metalsmith/markdown lib/extract-page-headings lib/get-macro-options lib/marked-renderer

Why

@metalsmith/markdown uses marked v4+ under the hood, and this does the heavy lifting of rendering our markdown code to HTML.

However, we also directly depend on marked for some of our plugins which we use to define custom renderers and lexers.

Recently, dependabot tried to bump this direct dependency to the new v5, which resulted in a swathe of deprecation warnings for the mangled and headerIds options.

mangled

We set mangled to false via @metalsmith/markdown, but not in our plugins, so even though we don't need mangling in these plugins, we were still deprecation warnings because we hadn't explicitly disabled it.

headerIds

We use this property to generate ids for our headings. I looked into use the plugin marked recommends instead, but it generated ESM-related errors: https://github.com/markedjs/marked-gfm-heading-id/issues/310

We want to avoid getting warnings or errors like these that only apply to one part of the build process.

Who needs to work on this

Developer

Who needs to review this

Developer

Done when

webketje commented 1 year ago

Since v1.9.0 @metalsmith/markdown provides a render option which you could use to force using a locally installed marked version: https://github.com/metalsmith/markdown#using-another-markdown-library. Since v1.8.0 the options have moved to engineOptions, see https://github.com/metalsmith/markdown/releases/tag/v1.8.0 (this warning is logged when metalsmith.env('DEBUG') contains @metalsmith/markdown*)

I may consider making marked an (optional) peerDependency in a future major release due to the complications of their switch to extensions vs options (tracked in https://github.com/metalsmith/markdown/issues/70)

colinrotherham commented 1 year ago

Just a note to say marked@8.0.0 has been released so deprecated options have been removed

Breaking changes

Heading IDs from Metalsmith's marked@4.3.0 are removed in DesignSystemRenderer() using marked@8.0.0

We've seen this before, since in https://github.com/alphagov/govuk-design-system/pull/3002/commits/21787a8a593b5487d1ba0a64aed58edbb7cc8c5f we noted that jstransformer-marked still needed { smartypants: true } but elsewhere the option was logging deprecation warnings so needed marked-smartypants instead

Using the suggestion above though we could bypass Metalsmith's marked@4.3.0 rendering with:

const markdown = require('@metalsmith/markdown')
const { Marked } = require('marked')

// Use `marked@8.0.0`
const marked = new Marked()

metalsmith.use(
  markdown({

    // Bypass `marked@4.3.0`
    render(source) {
      return marked.parse(source)
    }
  })
)

Deprecated options removed

The renderer bypass would need to use the following packages instead of options:

Since the previous options have now been removed (see known extensions list)

colinrotherham commented 1 year ago

Turns out { renderer: DesignSystemRenderer() } does override by design

From the documentation:

The Marked Pipeline

For example, using marked.use({renderer}) would modify a render, whereas marked.use({extenstions: [{renderer}]}) would add a new renderer.

But handling tokens (not strings) with named extensions lets each one keep their own new renderer:

Ready for review

36degrees commented 1 year ago

Looks like this is done 🎉