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: add mdx support #256

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

feat: add mdx support #256

wants to merge 9 commits into from

Conversation

I-3B
Copy link

@I-3B I-3B commented Jan 29, 2024

closes #211

This line may causes problem if some users where only using doctoc for .md files, I didn't check for it, should I?

@I-3B I-3B marked this pull request as draft January 29, 2024 20:08
@I-3B I-3B marked this pull request as ready for review January 29, 2024 20:08
@I-3B
Copy link
Author

I-3B commented Jan 29, 2024

@AndrewSouthpaw would you please review this?

@chris-dura
Copy link

This line may causes problem if some users where only using doctoc for .md files, I didn't check for it, should I?

So, if I understand, this means that users would all of a sudden start getting TOCs in their .mdx files, which they weren't before... I could see how this would be considered a "breaking change" at worst, and annoying at best to users 😅.

I don't think it's worth forcing a MAJOR version bump, so to keep it backwards compatible, you may want to make the MDX support "opt-in", so the doctoc command does the same thing by default, and you have to explicitly enable mdx support?

@I-3B
Copy link
Author

I-3B commented Jan 30, 2024

This line may causes problem if some users where only using doctoc for .md files, I didn't check for it, should I?

So, if I understand, this means that users would all of a sudden start getting TOCs in their .mdx files, which they weren't before... I could see how this would be considered a "breaking change" at worst, and annoying at best to users 😅.

I don't think it's worth forcing a MAJOR version bump, so to keep it backwards compatible, you may want to make the MDX support "opt-in", so the doctoc command does the same thing by default, and you have to explicitly enable mdx support?

I can think of two approaches:

  • .mdx is only matched if --syntax mdx is provided, but would also still match .md
  • the more expected behavior, only match .mdx if --syntax mdx and only match .md and .markdown if --syntax md (default)

@chris-dura
Copy link

I like what you're doing with a --syntax argument better... but I'm an idiot and must've forgot to open a PR from my fork where I implemented this for a local project, just for reference:

#241

Anyway, not something I thought about in mine, but, I would allow --syntax to support just .md/.markdown (default), just .mdx, or "all syntaxes". Then, the default behavior is unchanged, but you can opt-in to doing .mdx files, or all of them?

@AndrewSouthpaw
Copy link
Collaborator

That seems reasonable to me. Not breaking the default behavior is important.

@I-3B
Copy link
Author

I-3B commented Apr 27, 2024

@AndrewSouthpaw hi!

fixed the issue mentioned before here a6ee89c, so now mdx files won't get matched unless --syntax mdx is explicitly specified.

@AndrewSouthpaw
Copy link
Collaborator

Hi there! Sorry it took so long to get back to you. The code looks good, all it needs now is some test coverage and we can ship it! 🥳

@I-3B
Copy link
Author

I-3B commented May 24, 2024

Hi~ @AndrewSouthpaw
added some tests, should cover the new change, would you like me to add more?

Copy link
Collaborator

@AndrewSouthpaw AndrewSouthpaw left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry for a few more comments, I did one more thorough review, and just a few tweaks would make this code cleaner. Thanks for the additional tests. 😄

@thlorenz would you want to bump and publish the new version number, or should I take care of that? This seems like a minor version bump.

lib/transform.js Outdated Show resolved Hide resolved
lib/transform.js Outdated Show resolved Hide resolved
doctoc.js Outdated Show resolved Hide resolved
lib/file.js Outdated Show resolved Hide resolved
lib/file.js Outdated Show resolved Hide resolved
test/transform.js Outdated Show resolved Hide resolved
@I-3B
Copy link
Author

I-3B commented Jun 3, 2024

Hi @AndrewSouthpaw !
thanks for the thorough review, can you please go through the new commits to check if they cover the review?

doctoc.js Outdated Show resolved Hide resolved
@AndrewSouthpaw
Copy link
Collaborator

Perfect! One quick lint cleanup and this is good to go. Thanks @I-3B.

Co-authored-by: Andrew Smith <[email protected]>

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 7 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • test/fixtures/readme-syntax.mdx: Language not supported
  • lib/file.js: Evaluated as low risk
Comments skipped due to low confidence (4)

test/file.js:26

  • The test case for undefined syntax should use undefined instead of 'md' to accurately test the default behavior.
_(findMarkdownFiles(fixturesDir, 'md')).every((file) => file.path.endsWith('.md'))

lib/transform.js:22

  • The start and end properties of the exports object are set to startMd and endMd, which could cause confusion if the syntax is changed dynamically. Ensure that the start and end properties are updated correctly based on the syntax.
var { start: startMd , end: endMd } = generateComments('md')

lib/transform.js:49

  • The generateComments function modifies the exports object directly. Ensure that the function does not modify the exports object to avoid unexpected behavior.
function generateComments(syntax){

lib/transform.js:24

  • The matchesStart and matchesEnd functions are defined within the transform function, which could lead to performance issues if the transform function is called frequently. Move these functions outside of the transform function to avoid redefining them multiple times.
function matchesStart(syntax) {
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.

Support for .mdx files
3 participants