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

Unable to define settings.handlers with non-structuredClone-able values #77

Closed
4 tasks done
stern-shawn opened this issue Apr 23, 2024 · 5 comments
Closed
4 tasks done
Labels
💪 phase/solved Post is done 🐛 type/bug This is a problem

Comments

@stern-shawn
Copy link

stern-shawn commented Apr 23, 2024

Initial checklist

Affected packages and versions

[email protected], remark-lint

Link to runnable example

No response

Steps to reproduce

  • Create a project and set up remark-lint, configured using a remarkrc.js file.
  • In the exported remark config object, define settings.handlers and provide the expected key-value pairs for nodes such as text, paragraph, etc
  • Specifically for text, try to take advantage of search and replace support from remark-stringify by defining some regex + replace values:
import remarkFrontmatter from 'remark-frontmatter';
import remarkGfm from 'remark-gfm';
import remarkLintNoEmptyURL from 'remark-lint-no-empty-url';
import remarkLintNoHtml from 'remark-lint-no-html';
import remarkMdx from 'remark-mdx';
import remarkDirective from 'remark-directive';
import presetLintRecommended from 'remark-preset-lint-recommended';
import {
  paragraph,
  remarkCustomHeadingId,
  remarkLintHttpBackticks,
  remarkLintImages,
  remarkLintRedirects,
  remarkLintSearchReplace,
  remarkLintValidLocalLinks,
  text,
  link,
  remarkLintValidHeadings,
} from './dist/packages/remark/src/index.js';

export const SEARCH_REPLACE_RULES = [
  {
    name: 'curly-double-quotes',
    message: "Don't use curly double quotes",
    regex: /“|”/,
    replace: '"',
  },
  {
    name: 'curly-single-quotes',
    message: "Don't use curly single quotes",
    regex: /‘|’/,
    replace: "'",
  },
  {
    name: 'nbsp',
    message: "Don't use no-break spaces",
    regex: / /,
    replace: ' ',
  },
  {
    name: 'm-dash',
    message: "Don't use '--'. Use m-dash — instead",
    search: ' -- ',
    replace: ' — ',
  },
  {
    name: 'double-spaces',
    message: 'Avoid double spaces',
    regex: /(?<$1>[^\s>])\s{2,}(?<$2>[^\s|])/,
    replace: '$1 $2',
  },
  {
    name: 'incorrect-spelling',
    message: 'Incorrect spelling',
    regex: /e-mail/,
    replace: 'email',
  },
  {
    name: 'incorrect-spelling',
    message: 'Incorrect spelling',
    regex: /(?<$1>w)eb site/,
    replace: '$1ebsite',
  },
  {
    name: 'relative-link',
    message: "Internal links should start with '/'",
    regex: /\[(?:.*?)\]\((?:internal\/.*?)\)/,
    replace: '[$1](/$2)',
  },
];

export const remarkConfig = {
  extensions: '.mdx',
  plugins: [
    remarkFrontmatter,
    presetLintRecommended,
    remarkGfm,
    remarkMdx,
    remarkDirective,
    remarkCustomHeadingId,
    remarkLintNoEmptyURL,
    remarkLintNoHtml,
    remarkLintHttpBackticks,
    remarkLintImages,
    ['remark-lint-no-undefined-references', { allow: [/#[a-zA-Z]/] }],
    ['remark-lint-checkbox-character-style', { checked: 'x', unchecked: ' ' }],
    ['remark-lint-blockquote-indentation', 2],
    ['remark-lint-emphasis-marker', '*'],
    ['remark-lint-heading-style', 'atx'],
    ['remark-lint-link-title-style', '"'],
    ['remark-lint-ordered-list-marker-style', '.'],
    ['remark-lint-strong-marker', '*'],
    ['remark-lint-table-cell-padding', 'padded'],
    ['remark-lint-list-item-bullet-indent', false],
    ['remark-lint-list-item-indent', 'one'],
    ['remark-lint-first-heading-level', 2],
    ['remark-lint-ordered-list-marker-value', 'ordered'],
    ['remark-lint-unordered-list-marker-style', '-'],
    ['remark-lint-rule-style', '---'],
    ['remark-lint-no-literal-urls', false],
    'remark-lint-list-item-content-indent',
    'remark-lint-fenced-code-flag',
    'remark-lint-no-duplicate-headings',
    'remark-lint-no-heading-indent',
    'remark-lint-no-heading-like-paragraph',
    'remark-lint-no-empty-sections',
    ['remark-lint-no-emphasis-as-heading', false],
    ['remark-lint-no-duplicate-headings', false],
    ['remark-lint-no-undefined-references', false],
  ],
  settings: {
    bullet: '-',
    bulletOrdered: '.',
    listItemIndent: 'one',
    rule: '-',
    handlers: {
      paragraph,
      text: text(SEARCH_REPLACE_RULES),
      link,
    },
  },
};

export default remarkConfig;

Expected behavior

Running eslint should run without errors and correctly highlight instances where the find and replace rules apply in targeted markdown/mdx files. This configuration previously worked when we were on MDX v2 (and [email protected]), but now that we're updating plugins to work with MDX v3 and unified@11.* this has come up.

Actual behavior

We get an unexpected error:

error Cannot process file
  [cause]:
    Error: Cannot parse given file `.remarkrc.js`
    at file:///Users/sstern/Documents/trashfire/internal/node_modules/unified-engine/lib/find-up.js:137:19
    at done (file:///Users/sstern/Documents/trashfire/internal/node_modules/trough/index.js:148:7)
  [cause]:
    DataCloneError: function(node, _, state, info) {
  return defaultHandlers.paragraph(splitAndMapParagraphText(node, VERBS_REGEX,...<omitted>...
} could not be cloned.
    at new DOMException (node:internal/per_context/domexception:53:5)
    at structuredClone (node:internal/structured_clone:23:17)
    at file:///Users/sstern/Documents/trashfire/internal/node_modules/@ungap/structured-clone/esm/index.js:20:46
    at addPreset (file:///Users/sstern/Documents/trashfire/internal/node_modules/unified-engine/lib/configuration.js:352:23)
    at async merge (file:///Users/sstern/Documents/trashfire/internal/node_modules/unified-engine/lib/configuration.js:328:5)
    at async Configuration.create (file:///Users/sstern/Documents/trashfire/internal/node_modules/unified-engine/lib/configuration.js:266:7)

Looking at the source for configuration.js at that line, I see that this change was made in v11.2: 65da801#diff-c47540330d377e9cd0e6b633a168ffa74b2781e7a7f14ab6a8e934bd39576e22L265

- target.settings = Object.assign({}, target.settings, result.settings)
+ target.settings = structuredClone({...target.settings, ...result.settings})

I think this stylistic change may have unintentionally broken this element of support for settings.handlers, as functions cannot be cloned.

If this is the case and it would not break anything, I would be happy to open a PR to revert back to normal Object.assign or another object merge method that would still preserve functions.

Affected runtime and version

[email protected]

Affected package manager and version

[email protected]

Affected OS and version

No response

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 23, 2024
@wooorm
Copy link
Member

wooorm commented Apr 24, 2024

Hi Shawn!

I’m not sure that you should use the text handler for such things? 😬 Using a regular plugin and looking through text would be the recommended way?

extensions: '.mdx',

This doesn’t do anything in a .remarkrc.js btw. That key isn’t used.

I think this stylistic change may have unintentionally broken this element of support for settings.handlers, as functions cannot be cloned.

Indeed, functions should be supported. Kinda sad that they aren’t!

@wooorm wooorm closed this as completed in e9b1e0a Apr 24, 2024
@wooorm wooorm added 🐛 type/bug This is a problem 💪 phase/solved Post is done labels Apr 24, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Apr 24, 2024
@wooorm
Copy link
Member

wooorm commented Apr 24, 2024

Released in 11.2.1!

I noticed this in unifiedjs/unified earlier, where a similar algorithm is in place. But when I solved that, I didn’t think about the code here.
Now, I modelled the code here to match that code there.

@stern-shawn
Copy link
Author

stern-shawn commented Apr 25, 2024

Thank you so much for the prompt help, Titus! Using "overrides" lets us force this version in our dependencies and everything is working again 🎉

I see that this exists in a few places, so I've done my best to try and open some PRs in those repos to add this change 😅

❯ npm ls unified-engine
@docs/[email protected] /Users/sstern/trashfire/docs
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected] overridden
└─┬ [email protected]
  └─┬ [email protected]
    └── [email protected] deduped

@ChristianMurphy
Copy link
Member

@stern-shawn in each of those examples, the there is a version range, no update is needed upstream for you to get the fix
Either:

  1. Run npm update unified-engine on your repository for a more targeted updated
  2. or clear the package-lock.json file and reinstall the dependencies to update everything within range

@stern-shawn
Copy link
Author

Ah, sorry for the confusion there on the PRs then, thank you @ChristianMurphy !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done 🐛 type/bug This is a problem
Development

No branches or pull requests

3 participants