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

feat: Upgrade to Remark v13 with plug-ins #159

Merged
merged 3 commits into from
Nov 3, 2020
Merged

Conversation

nschonni
Copy link
Member

Some of the plug-ins require v13, so this bumps all the outstanding ones and the CLI

@nschonni
Copy link
Member Author

Upstream PR nodejs/node#35647

Need to figure out about the new requirement for remark-gfm to parse the tables now. I'm not sure if this needs to be added as a direct dependency/plug-in of the preset, or a change in the way it's called upstream

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once necessary changes land in nodejs/node repo

@nschonni
Copy link
Member Author

@Trott should the remark-gfm be added here as a direct dependency, or is that something to get bundled on the rollup setup in the node repo?

@nschonni nschonni marked this pull request as draft October 15, 2020 16:07
@aduh95
Copy link
Contributor

aduh95 commented Oct 16, 2020

Ideally it would be a peerDependency, right?

@Trott
Copy link
Member

Trott commented Nov 3, 2020

Since it's only needed for testing, I added it as a devDependency when I (unintentionally) duplicated this work in #161. Feel free to do that if you want to get this unstalled....

@Trott
Copy link
Member

Trott commented Nov 3, 2020

Oh, I see you already have it as a devDependency. I think you need to also add -u remark-gfm to the script line in package.json that calls remark.

@nschonni
Copy link
Member Author

nschonni commented Nov 3, 2020

@Trott how would you see that working upstream? Would you vendor the plug-in separately?

Some of the plug-ins require v13, so this bumps all the outstanding ones and the CLI
@nschonni
Copy link
Member Author

nschonni commented Nov 3, 2020

Oh, I see you already have it as a devDependency. I think you need to also add -u remark-gfm to the script line in package.json that calls remark.

It's there, just at the end instead of the front 😉

@Trott
Copy link
Member

Trott commented Nov 3, 2020

@Trott how would you see that working upstream? Would you vendor the plug-in separately?

It wouldn't come with the plugin. People could choose to use it or not use it, depending on if they were using GFM in their markdown files or not. In the case of Node.js core, we're using it so we include it. But that happens where you install remark-lint, not in the plugin itself. See the changes to tools/node-lint-md-cli-rollup/package.json and tools/node-lint-md-cli-rollup/src/cli-entry.js in https://github.com/nodejs/node/pull/35905/files.

@nschonni nschonni marked this pull request as ready for review November 3, 2020 03:28
@nschonni
Copy link
Member Author

nschonni commented Nov 3, 2020

Sounds, good. I know we had to use the frontmatter plug-in before for nodejs.org separately, but wasn't sure since there are rules were using seem to require the gmf plug-in (checkbox-style, etc...)

@Trott
Copy link
Member

Trott commented Nov 3, 2020

Sounds, good. I know we had to use the frontmatter plug-in before for nodejs.org separately, but wasn't sure since there are rules were using seem to require the gmf plug-in (checkbox-style, etc...)

I guess if there's a way to set GFM in the plugin, then great, but I don't know any way offhand, and we can always add it in later. But for now, it's on the consumer to know if they use/need GFM or not. I'm more than OK with that.

@Trott Trott merged commit ea3a7a5 into nodejs:master Nov 3, 2020
@Trott
Copy link
Member

Trott commented Nov 3, 2020

Because this might be incompatible with existing setups, I'm going to be extra cautious and publish it as a semver-major (breaking) change.

@nschonni nschonni deleted the remark-v13 branch November 3, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants