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

feature/code builder #77

Merged
merged 48 commits into from
May 24, 2022
Merged

Conversation

yankeeinlondon
Copy link
Collaborator

@yankeeinlondon yankeeinlondon commented May 24, 2022

@antfu another apology about the size of this PR, let me break it down for you:

Core

  • add comprehensive tests for bug fixes found in 0.13.1
  • closes issue Is there way to use frontmatter variable as image path? #66
  • upgraded the "build pipeline" which builders use to be asynchronous
    • as long as anyone had created their own builders external to this repo with createBuilder() ... which is recommended ... then it's fully backwards compatible
    • existing built-in builders do use createBuilder() and so no problems there

Happy Wrapper

  • while working with happy-dom I found myself wanting a more functional interface to it's API

  • currently it resides here as a separate "package" but the intent is to extract it as it's own package once the rate of change slows down (which actually it largely has now)

  • I have to say I was surprised about how shit I was at using the DOM API directly but having gone through this exercise I feel much better and the functional surface of happy-wrapper can be quite nice when working inside a transformation pipeline:

    pipe(
          fence.pre,
          addClass(`language-${fence.lang}`),
          setAttribute('data-lang')(fence.requestedLang),
          addClass(fence.props.class || ''),
          fence.props.style
            ? setAttribute('style')(fence.props.style)
            : identity,
          (fence.modifiers.includes(Modifier['!']) && !p.options.escapeCodeTagInterpolation)
        || (!fence.modifiers.includes(Modifier['!']) && p.options.escapeCodeTagInterpolation)
            ? setAttribute('v-pre')('true')
            : identity,
     )

Meta Builder

  • implemented the issue discussed in Set route with frontmatter variable? #72 ... there may still some parts which need tweaking but all tests pass currently
  • fixed a small issue with users who use both useHead and meta to make them work and tests are included

Code Builder

This works to 95% but I'm a bit stumped on getting to 100%. I noticed that vitepress/vuepress what feels like a hacky way to get line numbers incorporated into code blocks but there may be a good reason to use this styling. I have a builder option called layoutStructure which can be toggled between "tabular" and the default of "flex-lines". A tabular approach felt like a much more natural approach to doing this and so I've spent most of my time on this layout only to find no good way to preserve left-hand whitespace in most lines. Not sure why this is yet but it's taking enough time I'm considering falling back to the vitepress approach.

You can see the tabular approach in the example app as that's what I've relied on for visual testing recently.

  • this is the main area of change in this PR
  • adds code highlighting using PrismJS
  • I had originally added some abstractions for choosing between PrismJS and Shiki but this took longer than I had budgeted for so most of that abstraction has been extracted now ... I would be interested in whether you feel Shiki is a worthwhile addition in the future
  • currently i'm injecting CSS class definitions inline into pages -- which have code blocks -- and haven't really gotten a good unocss pattern in place but that's the plan going forward.

Testing

  • added a full test suite for the newly added code() builder
  • improved code coverage in core functionality
  • added code coverage report
    image
  • added the beginnings of some Cypress tests (but ran into problems so not really helpful yet but isolated from vitest so should cause no harm)

Markdown Linting?

To do some fixture testing I added a .markdownlint.json file and excluded some rules regarding the mixing of title and
H1 elements and that worked until I noticed a few cases where I see that ESLint appears to be complaining about MD files. Here's an example:

image

I didn't know that ESLint could cover MD scopes but maybe it can. Anyway, would like like a little advice how I should handle this.

Next Steps

I will create another PR that cleans up the remaining formatting issues with the code builder and hope to work in the sourcemap support into that but I think this one can be released "as is" as the core functionality is only improved.

yankeeinlondon and others added 30 commits March 7, 2022 14:56
feature: added better type hinting for Meta properties and Frontmatter properties

fix: fix type errors found in "head.ts"
- refactored to better isolate builders and include clearer pipeline operation
…to ensure full engagement with useHead functionality
…ality,

and the frontmatter pre-processor

refactor: improvements to happy-wrapper library and use of these improvements to
improve the code builder's transform pipeline
chore: add test coverage testing
refactor: removed shiki code
chore: attempts to get Cypress component tests working
- fix: fix type errors found in "head.ts"
- test: cypress testing works in very limited way but not when running in "tabular layout"
- test: vitest has been isolated from cypress symbol collisions
- fix: removed cypress cruft from their install process
- feature: added better type hinting for Meta properties and Frontmatter properties

Co-authored-by: Anthony Fu <[email protected]>

chore: update deps

docs: update README.md (antfu#68)

docs: import quote (antfu#70)

fix: loss information from <script> in md

chore: fix typo (antfu#73)
@yankeeinlondon
Copy link
Collaborator Author

Note: I have no idea why no types for happy wrapper are visible to CI:CD but since I was going to extract that as a separate repo anyway, i'll just do that now.

@yankeeinlondon yankeeinlondon requested a review from antfu May 24, 2022 17:27
@@ -0,0 +1,7 @@
lockfileVersion: 5.3
Copy link
Owner

Choose a reason for hiding this comment

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

What is this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm. Not sure. Must have been an old pnpm link i'll get rid of it.

@antfu
Copy link
Owner

antfu commented May 24, 2022

I am good with it as long as you believe it's the right thing to do.

Overall I am a bit concerned about the pipelines added to the repo, isn't that a bit like we are building another plugin system on top of markdown-it? Do you think it would be better to extract the builder into a standalone library so many other integrations could benefit from it as well? (just an idea, we could merge it as-is and do this for later, as you prefer)

@yankeeinlondon
Copy link
Collaborator Author

There's no question there is some overlap in utility with markdown-it's plugin eco-system but I find it more complimentary than competitive. I also think the core plugin had the beginnings of a callback based hook system which I assumed developed based on userland demand(although there also seemed to have some ties into core functionality demands). Either way it was growing organically and as a result had inconsistent semantics and naming. I think this pipeline -- which can be entirely ignored by users who don't need it -- helps to enforce consistency in naming and approach. As a "for instance" of this, the frontMatterPreprocess parameter appears to grow out of the need to work fluently with @usevue/head whereas it's utility is in providing a hook by another name. In contrast the transforms property seems like the beginnings of a more formal and traditional hook transform pipeline but to maintain backward compatibility and increase the power of the hook interface I went with a different route for the new pipeline.

Now the new pipeline simply incorporates the before/after props of the transforms property into the first and last steps of its pipeline. In a perfect world maybe we'd deprecate these transform hooks but I don't think that's necessary as I don't think they provide much cognitive load to understand how they fit into the broader transform pipeline.

Bear in mind, the "scope" of this new pipeline transform is intended to allow interested actors to intercept at the appropriate part of the transformation pipeline which includes the transformation of MD to HTML but also includes up and downstream transforms as well. A Markdown-IT plugin is purely focused on the MD-to-HTML transformation. This is an important part of the pipeline but not the only part. I have also found that while markdown-it has good docs and decent types, for many developers there's a bit of friction in getting their heads around how to tap into this ecosystem (other than just pulling plugins off the shelf).

I do think that the remaining "builders" very likely should be created as external repos. An external builder has precisely the same access as an internal one. The internal ones just provide reference and convenience. I had planned on creating two more smallish builders in the short term:

  • List Builder - provides open/close functionality to nested lists as well as providing nesting-level classes to aid with presentation/styling)
  • TOC Builder - provides the TOC structure to the frontmatter system to be used on page where appropriate but also as tokenized structure it can be used to feed navigation components as well as search engines

I had kind of gotten in the habit of thinking of these as "built-in" because I feel they have high reuse potential but if you prefer I'm happy to develop these externally. Maybe then we can reference them in the docs as examples of how people can add their own.

@yankeeinlondon
Copy link
Collaborator Author

I also wonder how much of the pipelines is "just in my head" versus described in a manner others can follow in the docs. I think it might be worth getting the docs a bit more touched up to accurately reflect my thinking and I'm hoping that this might actually make you feel better about this pipeline as I think this plugin is awesome and could be a fundamental building block but I do think it needs something like a formal mutation pipeline to allow it to grow into what it needs to be for various people.

@antfu
Copy link
Owner

antfu commented May 24, 2022

Thanks for the explanation! I like the idea of your transforming pipeline and I do think they are more flexible. I am totally fine to have them inside the repo, or move to a package. The benefit I see of having them as a package is that:

  • It could be a general builder/pipeline system that might be able to run framework agnostically (not only Vite) and probably even browser runtime. Which might benefit others as well.
  • Could have a clearer scope to document and explain the idea of builders, and even have a few sub-plugins from the community.
  • You got better credits for your work on it.
  • You have better control over it and how it release, and can release it independently from this integration.

That said, just an idea and it's totally up to you. No pressures.

Feel free to merge and release it when you think it's ready. Thanks.

@yankeeinlondon
Copy link
Collaborator Author

I'll merge this in; have raised a tiny PR which just updates the description of the Builder API a bit. Would love your feedback if you have time.

@yankeeinlondon yankeeinlondon merged commit db215b3 into antfu:main May 24, 2022
@yankeeinlondon yankeeinlondon deleted the feature/code-builder branch May 24, 2022 19:56
@azrikahar azrikahar mentioned this pull request Jun 17, 2022
8 tasks
@antfu
Copy link
Owner

antfu commented Jun 18, 2022

@yankeeinlondon Do you use Discord? You can add me with antfu#3212

@yankeeinlondon
Copy link
Collaborator Author

yankeeinlondon commented Jun 18, 2022 via email

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