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

Add remark-abbr to tests and re-record snapshots #515

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

richardTowers
Copy link

The only difference in the snapshots seems to be the presence of \\ backslashes before quote characters.

I'm a little confused by the output, not being all that familiar with jest, but I suspect this is some minor difference in the way that rehype-stringify works and that we don't need to worry about it?

This is just laying a small bit of groundwork, before I attempt to get remark-abbr working with the new version of remark

See also #514

@StaloneLab
Copy link
Member

I'm way too surprised that the tests work (apart from these backslashes, I'll check). It seems that actually the plugin might work out of the box on new remark versions: it relies only on unist-util-visit which operates only on AST and does not require anything special. I think you would have noticed if it was the case, what am I missing? Or maybe you wanted to use the low-level micromark API?

@StaloneLab
Copy link
Member

Hum I see unrelated linting problems, I fixed on next, you should be able to rebase (I forced push so maybe cherry-pick is easier). On top of that, there are some other changes to be done which we do while transitioning to next. I think the easiest way is to merge and I'll do the changes right after. They are stupid changes anyway, purely workflow, not really relevant to what you are trying to achieve.

The only difference in the snapshots seems to be the presence of `\\`
backslashes before quote characters. I'm a little confused by the
output, not being all that familiar with jest, but I suspect this is
some minor difference in the way that rehype-stringify works and that we
don't need to worry about it?
@richardTowers
Copy link
Author

Haha 🤣

I didn't try the version on the next branch, I just tried the version published to npm, discovered that it didn't work, and then found the issue talking about support for remark@13 and assumed it would need significant rework.

I didn't even bother to read the code properly, I just jumped straight in to building my own plugin from scratch. So it's a suprise to me that it works too! (but only a suprise because I made bad assumptions).

As you say, if the plugin does everything at the AST level, it shouldn't be bothered by the change in parser. So hopefully a minimal bit of tidy up and we're all good.

My odyssea of dodgy assumptions so far:

  • I assumed that this plugin needed a lot of rework, so I didn't spend a lot of time understanding it, I just jumped in to rewriting it
  • I initially assumed that The Right Way To Do It would be to parse everything at the micromark level, and then do pretty minimal work on the AST. I had a discussion about that with the micromark folks, which concluded that micromark is missing a couple of extension points that would enable parsing a new kind of definition (like abbreviation definitions are), and that I'd be best doing some of the work at the AST level.
  • I assumed that I had to parse abbreviation definitions at the micromark level (rather than finding them in an AST transform), so I wrote a micromark extesnion for that, and just handled the abbreviations themselve by transforming the AST

I'll port some of my test suite over, and we can see if there's any reason to bother with micromark, or if this covers all the edge cases well enough already.

@StaloneLab
Copy link
Member

StaloneLab commented Sep 22, 2024

To be clear: I am not at all advocating for our plugin, I think the way it currently works by basically using Regexes on the AST is quite weird. So if your extension has a more "micromark-compatible" way of doing things, I think it would be nice to implement anyway.

Also, the version published on NPM should be quite (if not exactly) similar to what we have in the next branch today. So if you say this didn't worked, then I am definitely missing something. I think I have a test setup somewhere for remark, so I'll test in the upcoming days (maybe today?).

I read your code quite rapidly, so I might have misunderstood things, I will read properly on MR, but from what I understand:

  • micromark is used to parse abbr definitions (i.e. *[W3C]: thing), while;
  • a transformer is used to replace occurences of the abbr inside of text nodes.

Is this right? If not, can you correct me? [EDIT: seems to correspond to what you described above]

Regarding this MR, it looks ready to merge. I will once the current state of remark-abbr on remark > 13 will have been clarified (i.e. I will have tested the extension to see what doesn't work).

@richardTowers
Copy link
Author

Yeah, that's exactly correct. Micromark parses the abbreviation definitions, and then a transformer splits text nodes into [text, abbr, text, abbr, text] etc.

@StaloneLab
Copy link
Member

So… I tested, and in fact we do that today: using an (old-style) remark tokenizer and then a transformer that replaces inside of text nodes. Maybe you could get inspired by the old code. In any case I'll look at it when reviewing yours.

Merging this PR now, and will get the repo fully ready for your changes.

@StaloneLab StaloneLab merged commit 203a59a into zestedesavoir:next Sep 23, 2024
1 of 4 checks passed
@StaloneLab
Copy link
Member

And since I forgot to answer about the backslash changes: I do not think we care much for now, if it becomes reversed after your change you might change it again, I do not think I encountered it before when migrating plugins so I am not sure if expected or not.

@StaloneLab
Copy link
Member

Ok so now tests do not run: they were actually still run with the old version of remark. You can start integration whenever you want, everything is ready on my side. I also disabled or fixed other failing tests so your objective is to get back to CI green check.

@richardTowers richardTowers deleted the next-enable-abbr-tests branch September 27, 2024 07:51
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.

2 participants