remark-retext icon indicating copy to clipboard operation
remark-retext copied to clipboard

Support string arrays like in unified-engine

Open remcohaszing opened this issue 4 years ago • 12 comments

Subject of the feature

It would be nice if remark-retext could support a unified plugin list instead of a processor using the same syntax as unified-engine. Logic for this resides in https://github.com/unifiedjs/unified-engine/blob/main/lib/configuration.js. It probably makes sense to extract it into a new unified project.

Problem

remark-retext is often used with remark-cli, which supports YAML and JSON configuration files, but remark-retext only works with JavaScript configuration files, because a processor is needed.

Expected behavior

It would be nice if the following .remarkrc file:

const dictionary = require('dictionary-en');
const english = require('retext-english');
const quotes = require('retext-quotes');
const repeatedWords = require('retext-repeated-words');
const syntaxURLs = require('retext-syntax-urls');
const unified = require('unified');

module.exports = {
  plugins: [
    'remark-frontmatter',
    'remark-gfm',
    'remark-lint-heading-increment',
    'remark-lint-no-duplicate-defined-urls',
    'remark-lint-no-duplicate-definitions',
    'remark-lint-no-empty-url',
    'remark-lint-no-reference-like-url',
    'remark-lint-no-undefined-references',
    'remark-lint-no-unneeded-full-reference-image',
    'remark-lint-no-unneeded-full-reference-link',
    'remark-lint-no-unused-definitions',
    'remark-prettier',
    ['remark-validate-links', { repository: 'https://gitlab.com/remcohaszing/koas.git' }],
    [
      'remark-retext',
      unified()
        .use(english)
        .use(syntaxURLs)
        .use(repeatedWords)
        .use(quotes),
    ],
  ],
};

could be rewritten as JSON or YAML:

plugins:
  - remark-frontmatter
  - remark-gfm
  - remark-lint-heading-increment
  - remark-lint-no-duplicate-defined-urls
  - remark-lint-no-duplicate-definitions
  - remark-lint-no-empty-url
  - remark-lint-no-reference-like-url
  - remark-lint-no-undefined-references
  - remark-lint-no-unneeded-full-reference-image
  - remark-lint-no-unneeded-full-reference-link
  - remark-lint-no-unused-definitions
  - remark-prettier
  - - remark-validate-links
    - repository: https://gitlab.com/remcohaszing/koas.git
  - - remark-retext
    - - retext-english
      - retext-syntax-urls
      - retext-repeated-words
      - retext-quotes

Alternatives

N/A

remcohaszing avatar Jul 22 '21 15:07 remcohaszing

Yes, it would be nice, but that would make this project impossible to use in browsers though. And I don’t see a good way around that (other than maybe a browser field, but not perfect?)

wooorm avatar Jul 24 '21 17:07 wooorm

that would make this project impossible to use in browsers though

Could you expand on why this would break browsers? Would it make sense/be possible to include a plugin loader on the this context for Plugins, which could abstract the node vs browser differences?

ChristianMurphy avatar Jul 27 '21 16:07 ChristianMurphy

I imagine something like this:

unified({
  resolver: (name) => loadPlugin(name, { 'remark' })
})
  .use(remarkRetext, [
    'retext-english',
    'retext-syntax-urls'
  ])

Then unified-engine could define the resolver. In a browser another resolver could be used. The default unified resolver should just throw an error, as it needs to be set explicitly.

The same approach could be used for retext-spell to load dictionaries from a path.

remcohaszing avatar Jul 28 '21 07:07 remcohaszing

I quite like that unified is as small (in size and API surface) as it can be, and this feels... complex.

  • remark-retext having to load these plugins at runtime is quite a waterfall (maybe those plugins will in turn also use .resolver, for spelling or for more plugins)
  • arbitrary file or network access could result in security vulnerabilities
  • rehype plugins would assume plugins with a rehype- prefix are loaded
  • Is this just for plugins? For files? Config files? Closest package.json?

wooorm avatar Jul 28 '21 08:07 wooorm

arbitrary file or network access could result in security vulnerabilities

I'm not sure I follow you you see this being different than the existing top level string based loader.

rehype plugins would assume plugins with a rehype- prefix are loaded

:thinking: makes sense, and interesting challenge.

Is this just for plugins? For files? Config files? Closest package.json?

I'd interpret it as being just plugins and presets.

ChristianMurphy avatar Jul 28 '21 13:07 ChristianMurphy

I'm not sure I follow you you see this being different than the existing top level string based loader.

Depends on what this can load. If dictionaries, then that seems arbitrary


This also makes configuration complex. The attacher is sync, but as this would operate on options and be async, it introduces a problem: user does .use(a, 'c').use(b), a is async and after a while will configure c after b, which is not what the user thought would happen. Furthermore, the user already called .parse / .run / etc, but c is unexpectedly omitted.

wooorm avatar Jul 29 '21 12:07 wooorm

Hi! Thanks for taking the time to contribute! This has been marked by a maintainer as needing more info. It’s not clear yet whether this is an issue. Here are a couple tips:

  • Spend time framing the issue! The more time you put into it, the more we will
  • Often, maintainers respond with why for several back and forths; rubber duck debugging might help avoid that
  • Folks posting issues sometimes fall for xy problems: asking for a certain solution instead of raising the root problem

Thanks, — bb

github-actions[bot] avatar Aug 07 '21 07:08 github-actions[bot]

jeez, finally GH fixed there label addition on the API bug. Sorry for all the noise this created though 😅

wooorm avatar Aug 07 '21 08:08 wooorm

I feel like the added complexity of this, including the headaches around things like endless loading and slow waterfalls, compared to the alternative: use a sharable preset / javascript file where ESM does all that nicely, makes me prefer the latter, current solution?

wooorm avatar Dec 30 '21 12:12 wooorm

I’d prefer to close this. Adding an fs/fetch loader here seems like a hassle. Using a .js config file / shared .js preset seems acceptable to me?

wooorm avatar Oct 27 '23 16:10 wooorm

I still think this would be nice to have. I think we actually don’t need the resolver. remark-retext could support this in Node.js, but not the browser, using export conditions.

remcohaszing avatar Oct 27 '23 16:10 remcohaszing

I still think this would be nice to have.

Why is the alternative, using .js, not acceptable? You get types in JS. You have import in Node that works everywhere.

think we actually don’t need the resolver

Then what do you propose?

remark-retext could support this in Node.js, but not the browser, using export conditions.

Feels off to me, to introduce different APIs in Node. Even though browsers/deno/etc also have import/import.meta.resolve? And our code would work slightly differently.

wooorm avatar Nov 06 '23 13:11 wooorm