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

Creating vs forking vs duplicating markdown-it plugins #1707

Closed
tlylt opened this issue Jan 12, 2022 · 5 comments · Fixed by #1786
Closed

Creating vs forking vs duplicating markdown-it plugins #1707

tlylt opened this issue Jan 12, 2022 · 5 comments · Fixed by #1786
Labels
a-Documentation 📝 s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding

Comments

@tlylt
Copy link
Contributor

tlylt commented Jan 12, 2022

Hi guys,

I am working on a version update for the markdown-it package and am curious what were the thoughts behind our approach in maintaining markdown-it packages. At the moment, it seems like there are three types of markdown-it plugins:

  1. plugins that we use as-is, and shown in package.json
  • e.g. markdown-it-task-lists
  1. plugins that we have modified/patched slightly, also shown in package.json:
  • e.g. markdown-it-attrs
  1. plugins that we copy the source code and keep in core/src/lib/plugins, not in package.json
  • e.g. markdown-it-center-text

Not too sure if there are any discussions on this before, but curious what were the considerations when deciding which approach to go for (maybe also in general)?

Some questions I have:

  • should we fork the original plugin and maintain it as a forked plugin?
    • especially when the original plugin (or packages like live-server) is no longer updated in a few years.
  • should we create individual markdown-it plugins for the new functionalities that we introduced?
    • especially if the plugin has a general purpose that could potentially benefit others
  • when should we decide to copy the source code and patch the code within our repo?
    • the potential pitfall from this is that version updates may become more troublesome since our patch is targeted towards a specific version of the package

Any discussions or thoughts on this would be appreciated!
p.s If this is a useful topic, perhaps I can help write it up in our documentation with some guidelines so that we can have a standardized approach to package management and allow future devs to know what to do when faced with such a choice:)

@jonahtanjz
Copy link
Contributor

Hi @tlylt,

I believed we used to fork some of the packages and maintain them separately such as htmlparser2 and vue-strap but decided to shift all of them to one repository at some point. I think this was to make it easier to maintain and track? Not entirely familiar with this as it happened quite some time ago, perhaps @ang-zeyu can chip in on this?

Any discussions or thoughts on this would be appreciated!
p.s If this is a useful topic, perhaps I can help write it up in our documentation with some guidelines so that we can have a standardized approach to package management and allow future devs to know what to do when faced with such a choice:)

I'm open to discussion on this and I feel this will be a good idea if we can come up with a proper workflow :)

@jonahtanjz jonahtanjz added the s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding label Jan 14, 2022
@ang-zeyu
Copy link
Contributor

should we fork the original plugin and maintain it as a forked plugin?

Tldr I don't have a clear conclusion on this unfortunately, I think its very much a case-by-case basis for each. 🙁

On fork vs patch

htmlparser2 was moved over to ensure cheerio uses receives our patched updates (see #606)

I suppose one reason we would want to maintain a patch as such is to ensure the changes propagate to other dependencies.

Agreed on the p.s., we could add a simple comment linking to this PR / other related issues in the patch file!


As for vue-strap, its essentially the pros (and cons) of a monorepo (also see #411)

Since the discussion is well documented around the web, we could link the mention of monorepo here in the dev guide to an article explaining this.


More generally I can think / summarise these reasons:

  • is the upstream still maintained? (less incentive to fork if not)
  • how big is the project? (e.g. live-server is just one file we could actually move over very easily (not the case right now though))
  • how invasive our changes are (e.g. vue-strap which is basically a rewrite).
    On one hand, the larger the more reason maintain it independently since at some point the upstream changes no longer benefit us.
    On the other hand, the larger the more reason to fork it since we can leverage the testing procedures of the original repo.
  • plugin system of the original project (highly dependent)
    e.g. markdown-it which provides the .at system for explicitly overriding the default rules

My opinion is also that forking (to patch some behaviours) does not make maintaining patches/etc. any easier than "patching" (by overriding exports), it is a difficult task to maintain the patched implementation against the upstream in either case.
E.g. you might have saw that in #1701 you will need to browse through the commit history of markdown-it, even from commits that don't change the files we directly patched. (it is never a simple case of git merge from upstream)

Other than that, I can think of:

Pros of a fork:

  • leverage upstream testing procedures

Pros of patching:

  • "quick fix": no need to maintain / publish more npm packages, setup release procedures, etc.
  • benefits of a monorepo

Another example are the nunjucks patches here which are the most extensive. At the time of those patches nunjucks had been dormant for quite a while (still quite is https://github.com/mozilla/nunjucks/commits/master); maintaining a fork seemed pointless hence. On the other hand I had to replicate the patched changes on the nunjucks repo to utilise its automated testing procedures to ensure default behaviours weren't changed, which was rather painful for development.
Side note: these specific patches pretty much went unreviewed as they are rather invasive changes of nunjucks + due to manpower constraints, I am entirely open to forking if there is a consensus!

@ang-zeyu
Copy link
Contributor

should we create individual markdown-it plugins for the new functionalities that we introduced?

As a separate npm package?
I think it would definitely be great if someone takes up the task of maintaining it externally (also depends if there is a demand for such plugins).
But otherwise, I'm not sure if there are any other benefits since markdown-it's plugin system is quite robust

when should we decide to copy the source code and patch the code within our repo?

Same as the first point, I think its very much a case-by-case basis for each. 🙁

@ang-zeyu
Copy link
Contributor

If this is a useful topic, perhaps I can help write it up in our documentation with some guidelines so that we can have a standardized approach to package management and allow future devs to know what to do when faced with such a choice

More generally on this, I think it would definitely be great to briefly list in the dev guide:

  • all the patches we have currently
  • general points for consideration
    • ultimately still assessed on a case-by-case basis

@tlylt
Copy link
Contributor Author

tlylt commented Feb 21, 2022

Hi @ang-zeyu @jonahtanjz, thank you for your kind input!
I agree that it is not that straightforward when making a decision about adding external dependencies. I have put together PR #1786 to summarize some of the points mentioned here, to help provide some points for consideration in our DG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Documentation 📝 s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants