pkp-lib
pkp-lib copied to clipboard
Reduce highlights.js bundle size
Describe the bug Recently added highlight.js dependency for JATS xml syntax rendering notably increased overall bundle size. To Reproduce Steps to reproduce the behavior:
-
run npx vite-bundle-visualizer
What application are you using? OJS, OMP or OPS version main branch (3.5)
Additional information Highlight.js supports lots of different languages, my understanding is that we need just xml. Documentation explains the option how to include only languages that we need. We probably also don't need vue3-highlight package which is very primitive and makes more difficult to configure highlights.js directly.
@defstat Hi Dimitris, could you have a look? Thank you!
@jardakotesovec thanks for the tips.
I added some proposed PRs in the issue's starting comment. I removed vue3-highlightjs and from highlight.js I used only XML language.
They are ready for review if you have some time.
The result seems promising:
@defstat Nice to see the size reduction. I made some suggestions, let me know what you think.
@defstat Nice to see the size reduction. I made some suggestions, let me know what you think.
Thanks @jardakotesovec. Please find new PRs at the initial comment of the issue.
@defstat That looks great. One thing that I am wondering about is - are we going to use that functionality in omp or ops?
If not maybe we could use this approach - https://github.com/highlightjs/vue-plugin?tab=readme-ov-file#using-component-locally , where we can still just import the xml highlighter and use the component from their plugin. We would have just simple wrapper component to be added in src/components to handle that.
Advantage would be that it would be inlucluded only in OJS, because it would be importing our component which imports the highlight stuff.
What you think?
@defstat That looks great. One thing that I am wondering about is - are we going to use that functionality in omp or ops?
If not maybe we could use this approach - https://github.com/highlightjs/vue-plugin?tab=readme-ov-file#using-component-locally , where we can still just import the xml highlighter and use the component from their plugin. We would have just simple wrapper component to be added in src/components to handle that.
Advantage would be that it would be inlucluded only in OJS, because it would be importing our component which imports the highlight stuff.
What you think?
@jardakotesovec thanks. Are you proposing that I add the reference to highlightjs-vue in the PublicationSectionJats.vue
? Or something else? I am asking because I thought that this comment proposed to use the code centrally - but maybe I misunderstood it.
@defstat If this would be (eventually) used also in omp&ops its fine to include it in load.js
But if its only for OJS, than best way to handle it centrally is just to create simple component (as in their example https://github.com/highlightjs/vue-plugin?tab=readme-ov-file#using-component-locally), which in template wraps their component and handles the imports. That would also result in importing it at one place and make it easy to use on other places than the PublicationSectionJats.vue.
No OMP and OPS is not using it right now.
I will try the approach you are proposing. Do you have a preference in which folder inside the src/components
to put the new one? I am thinking of naming it XMLCodeHighlighter.vue
.
@defstat What about src/components/CodeHighlighter/CodeHighlighter.vue ? Just in case we want to support more options in future :-).
ok @jardakotesovec! I added the possibility of selecting from a set of languages and pushed the code. We can either
- leave those,
- add only XML or
- add the
import hljs from 'highlight.js/lib/common';
instead ofimport hljs from 'highlight.js/lib/core';
.
The more languages we support, the bigger the lib size it gets. I leave this decision on you.
@defstat I think that bundle size will gets decided based on the imports in build time. Therefore importing any additional languages that we don't need would be unnecessarily adding to the overall bundle size.
So I would recommend to keep it minimal and just do the:
import xml from 'highlight.js/lib/languages/xml';
hljs.registerLanguage('xml', xml);
and we can extend that if needed. The registration is likely just connecting the language module with the engine, which I would expect be very low overhead, so I don't think it needs to be done conditionally.
For new components we are aiming to use composition api - https://main--6555d3db80418bb1681b8b17.chromatic.com/?path=/docs/guide-technical-roadmap--docs#vue3-composition-api-35 , could you update it? You can look for example to simple example of Button.vue to see how the props are defined there and components imported.
And ideally if you could create the mdx&story file for storybook? Just with little bit of documentation - it will help with awarness that we have such component. You can also follow button.vue how thats done. Feel free to reach out on mattermost if you need more guidance on that.
@jardakotesovec thanks. I think I concluded the review changes required and added the appropriate files for the StoryBook.
Pushed everything for your approval.
@jardakotesovec after finishing the review fixes, I have added PRs for OMP and OPS (see above) and waiting for the tests to pass - I have added the reference to highlight-vue
through:
npm uninstall vue3-highlightjs
npm install @highlightjs/vue-plugin@^2.1.0
After the tests pass, and if you have no other comments regarding the code, it can be merged
@jardakotesovec everything merged.
@defstat Excellent, thank you!