vuepress / ecosystem

Official plugins and themes for VuePress2
https://ecosystem.vuejs.press
MIT License
29 stars 41 forks source link

feat(helper): add helper package #33

Closed Mister-Hope closed 9 months ago

Mister-Hope commented 9 months ago

See #31

github-actions[bot] commented 9 months ago

Pull Request Test Coverage Report for Build 7691317854


Changes Missing Coverage Covered Lines Changed/Added Lines %
tools/helper/src/node/page/excerpt.ts 61 63 96.83%
tools/helper/src/node/utils/packageManager.ts 49 68 72.06%
tools/helper/src/node/bundler/vite/mergeViteConfig.ts 28 48 58.33%
<!-- Total: 180 221 81.45% -->
Totals Coverage Status
Change from base Build 7686894669: 12.6%
Covered Lines: 228
Relevant Lines: 296

💛 - Coveralls
Mister-Hope commented 9 months ago

@nruffing Could you please review it? I believe the code is at good quality, but need some suggestions about the API design, especially option names in params.

E.g.: I am using preserveTitle, preserveFenceDom, isCustomElement in excerpt options, but maybe they can have better names, for example using separator instead of excerptSeparator?

nruffing commented 9 months ago

@nruffing Could you please review it? I believe the code is at good quality, but need some suggestions about the API design, especially option names in params.

E.g.: I am using preserveTitle, preserveFenceDom, isCustomElement in excerpt options, but maybe they can have better names, for example using separator instead of excerptSeparator?

Is there a specific reason we dont want to allow Vue inside the excerpt or leverage Vue as the mechanism to implement the logic? For example, why couldn't we have a component with a default slot that does this same behavior?

Mister-Hope commented 9 months ago

For all of the 3 things, Vue component could not be rendered.

  1. For feed, absolutely we can not get Vue component rendered in feed readers.
  2. For seo, this is similar. We only need the text in excerpt.
  3. For blogging, unless we introduce vue compiler (which is 500kb - 4mb based on the features in use), we can not dynamically render a template string to dom. I believe in most situation, developer should use v-html directive and append the excerpt to dom via innerHTML, in this case we still need to remove Vue Components
nruffing commented 9 months ago

For all of the 3 things, Vue component could be rendered.

  1. For feed, absolutely we can not get Vue component rendered in feed readers.
  2. For seo, this is similar. We only need the text in excerpt.
  3. For blogging, unless we introduce vue compiler (which is 500kb - 4mb based on the features in use), we can not dynamically render a template string to dom. I believe in most situation, developer should use v-html directive and append the excerpt to dom via innerHTML, in this case we still need to remove Vue Components

Got it, that makes sense. Thank you for explaining the use cases.

Mister-Hope commented 9 months ago

It would be great if you can provide some suggestions about the export name of these helpers and their options, I am not good at giving names 🥺 Feel free to give any ideas about changing any name or shape.

Mister-Hope commented 9 months ago

For all of the 3 things, Vue component could not be rendered.

  1. For feed, absolutely we can not get Vue component rendered in feed readers.
  2. For seo, this is similar. We only need the text in excerpt.
  3. For blogging, unless we introduce vue compiler (which is 500kb - 4mb based on the features in use), we can not dynamically render a template string to dom. I believe in most situation, developer should use v-html directive and append the excerpt to dom via innerHTML, in this case we still need to remove Vue Components

And another important one, we need to do hacky things to get Vue components that is imported locally in page.🫠 So if someone really needs this, he might need to build a new pipeline to achieve this, and maybe make the excerpt "a vue component" instead of a html fragment.

nruffing commented 9 months ago

It would be great if you can provide some suggestions about the export name of these helpers and their options, I am not good at giving names 🥺 Feel free to give any ideas about changing any name or shape.

I left a couple of comments related to naming and comments but overall it looks good.

I am actually more wondering if the packaging organization makes sense. Do we really want a utility package of a bunch of random stuff? It just seems like it could grow to be a little out of control and maybe something like the excerpt functionality belongs in a package related to just SEO utilities?

Mister-Hope commented 9 months ago

I am planning to migrate vuepress-plugin-blog2 vuepress-plugin-feed2 vuepress-plugin-seo2 and vuepress-plugin-sitemap2 into this repo in the near future under @vuepress/xxx, and even more of my plugins.

They are designed to work with any theme, powerful and customizable, not just my theme hope.

So under this, we need a util package to store the common method of these plugins anyway.

And for excerpt, it is important (VuePress v1 core concerpt and also a core concerpt in VuePress 2 early stage), so I think it's worthy to add in the helper pacakge.

I don't plan to add uncommon ones into the helper package.

Mister-Hope commented 9 months ago

I am doing heavy refactor and improving the code quality in my repo in recent days, and once the helper package is ready, I will start shipping them.

Mister-Hope commented 9 months ago

I left a couple of comments related to naming and comments but overall it looks good.

BTW, I am still not seeing any comments here, are you still working on it, or did you forget to finish your review?

nruffing commented 9 months ago

I left a couple of comments related to naming and comments but overall it looks good.

BTW, I am still not seeing any comments here, are you still working on it, or did you forget to finish your review?

My bad, I never submitted the review.