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

Support string arrays like in unified-engine #14

Closed
remcohaszing opened this issue Jul 22, 2021 · 16 comments
Closed

Support string arrays like in unified-engine #14

remcohaszing opened this issue Jul 22, 2021 · 16 comments
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on 🦋 type/enhancement This is great to have

Comments

@remcohaszing
Copy link
Member

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

@wooorm
Copy link
Member

wooorm commented Jul 24, 2021

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?)

@ChristianMurphy
Copy link
Member

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?

@remcohaszing
Copy link
Member Author

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.

@wooorm
Copy link
Member

wooorm commented Jul 28, 2021

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?

@ChristianMurphy
Copy link
Member

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

🤔 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.

@wooorm
Copy link
Member

wooorm commented Jul 29, 2021

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 wooorm added 🙉 open/needs-info This needs some more info and removed 🔍 status/open labels Aug 7, 2021
@github-actions

This comment has been minimized.

@github-actions github-actions bot added the 🤞 phase/open Post is being triaged manually label Aug 7, 2021
@wooorm

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Dec 30, 2021

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
Copy link
Member

wooorm commented Oct 27, 2023

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?

@remcohaszing
Copy link
Member Author

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.

@wooorm
Copy link
Member

wooorm commented Nov 6, 2023

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
Copy link
Member

wooorm commented Feb 3, 2025

@remcohaszing Ping! :)

@wooorm
Copy link
Member

wooorm commented Feb 11, 2025

Closing, I do not think something needs to change here.

The OP is really about use of remark-retext through remark-cli using a non-JS config file.

Primarily, I think there is a good alternative: JavaScript is fine! An expressive language. JS has lots of tooling to bundle things, --loaders, support for pnpm/yarn whatnot in Node. There are other cases where JSON/YAML are not enough for users to describe their config files (such as regex).

Secondary, eyes on the ball, the place that loads things is the engine. It supports these non-JS languages and it loads plugin. If we want to change loading, then that is probably the place to change. Such as with a sort of “inline” preset. That could look like:

plugins:
  - remark-frontmatter
  - - remark-retext
    - plugins:
      - retext-english
      - retext-syntax-urls
      - retext-repeated-words
      - retext-quotes

So, allow an object with either or both plugins, settings fields and treat it as a “nested” “inline” config file.
It would mean adding a sort of “preset” support here in remark-retext, which I think is fine.
(Note: presets and config files are the same, with the exception that strings can be used instead of functions, which are loaded)

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Feb 11, 2025
@wooorm wooorm added the 🙅 no/wontfix This is not (enough of) an issue for this project label Feb 11, 2025

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually 🙉 open/needs-info This needs some more info labels Feb 11, 2025
@remcohaszing
Copy link
Member Author

Given we support JSON and YAML configuration files, I still believe it would be nice if remark configuration files can specify retext plugins as strings. However, I’m also leaning towards the idea that supporting multiple configuration file formats adds unnecessary complexity (unifiedjs/unified-engine#72 (comment)). Based on that and given JavaScript is the most powerful format we support, I think this issue is obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙅 no/wontfix This is not (enough of) an issue for this project 👎 phase/no Post cannot or will not be acted on 🦋 type/enhancement This is great to have
Development

No branches or pull requests

3 participants