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

Update marked in metalsmith build #2333

Closed
wants to merge 10 commits into from
Closed

Conversation

domoscargin
Copy link
Contributor

What

When we merged #2276, we realised we had generated several formatting issues across the site, so we reverted that change.

We use metalsmith-in-place to compile our markdown and nunjucks. That in turn has a dependency on jstransformer, which then requires peerDependencies of the relevant packages - in this case, jstransformer-marked and jstransformer-nunjucks.

So despite us having a fairly up to date marked as a dependency, we're at the whim of jstransformer-marked's dependency tree when it comes to processing our files with metalsmith-in-place.

Luckily, jstransformer-marked has been updated to use marked 4+. So a simple bump, right?

Wrong, dear reader. Follow along commit-by-commit for the whole sorry affair.

@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch from e6e174b to 646027d Compare August 24, 2022 23:47
@netlify
Copy link

netlify bot commented Aug 24, 2022

You can preview this change here:

Name Link
🔨 Latest commit e6e174b
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/6306b85bfd94d500084dfcce
😎 Deploy Preview https://deploy-preview-2333--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch 2 times, most recently from c2a4a51 to 64a71f8 Compare August 24, 2022 23:49
@netlify
Copy link

netlify bot commented Aug 24, 2022

You can preview this change here:

Name Link
🔨 Latest commit c2a4a51
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/6306b8e244d94c00094cbfa2
😎 Deploy Preview https://deploy-preview-2333--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 24, 2022

You can preview this change here:

Name Link
🔨 Latest commit 56c39b3
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/6310ac2bb2be410008fe37a4
😎 Deploy Preview https://deploy-preview-2333--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch 2 times, most recently from 5156ad3 to c603040 Compare August 25, 2022 07:25
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch 10 times, most recently from 65a29d2 to ef33d5c Compare August 31, 2022 08:43
@domoscargin domoscargin self-assigned this Aug 31, 2022
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch from dcf2adf to 48ea004 Compare September 1, 2022 10:16
Also stop file hashing so we don't have hashing diffs each time.

Also prevent email mangling for the same reason.

This allows comparisons between any future deploy builds and the current build.

It must be removed before merging!
Patch change with no major issues.

Of note is that this does NOT affect our build - `jstransformer-marked` 1.0.3 is using `marked` ^0.3.9 internally.
With pedantic rules, lists must have a preceding empty line. This rule is relaxed in Github Flavoured Markdown, but even if we switch to that, it'd be good to be as broadly compliant as possible.
Github Flavoured markdown supports naked links, but pedantic rules don't. So we wrap them with angle brackets.

This allows for broader compliance, so we could choose to stick with this even if we changed to GFM.
The calm before the storm...

We use `metalsmith-in-place` to compile our markdown and nunjucks. That in turn has a dependency on `jstransformer`, which then requires peerDependencies of the relevant packages - in this case, `jstransformer-marked` and `jstransformer-nunjucks`.

So despite us having `marked` 4.0.18 as a dependency, we're at the whim of `jstransformer-marked`'s dependency tree when it comes to processing our files with `metalsmith-in-place`.

v1.0.3 of `jstransformer-marked` uses `marked` ^0.3.9 (0.3.19 in our package-lock).

v1.2.0 uses `marked` 4.0.18.

Surely this won't be an issue... (spoiler: it will).
Take Nunjucks to 3.2.3 and drops support for Node < 8

This at least doesn't appear to affect any file processing.
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch 2 times, most recently from 501fd1a to 9e9fb5b Compare September 1, 2022 10:25
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch from 9e9fb5b to ba537ab Compare September 1, 2022 10:39
We remove `pedantic`, since `marked` defaults to `gfm` being true.
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch 5 times, most recently from c1c440c to ddd1664 Compare September 1, 2022 12:40
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch from ddd1664 to 53bb3d5 Compare September 1, 2022 12:53
@domoscargin domoscargin force-pushed the bk-metalsmith-marked-update branch from 53bb3d5 to 56c39b3 Compare September 1, 2022 12:57
@webketje
Copy link
Contributor

webketje commented Sep 7, 2022

@domoscargin if you had trouble with jstransformer-marked or @metalsmith/in-place please submit an issue in the relevant repo, I will take care of patching either/both. cf:

v1.0.3 of jstransformer-marked uses marked ^0.3.9 (0.3.19 in our package-lock).
v1.2.0 uses marked 4.0.18.
Surely this won't be an issue... (spoiler: it will).

@domoscargin
Copy link
Contributor Author

Hey @webketje, thanks for reaching out - it's not an issue with either package, thankfully - our markdown/nunjucks is just quirky because we've been using marked 0.3.9 for so long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants