pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
304 stars 444 forks source link

Reduce highlights.js bundle size #9840

Closed jardakotesovec closed 4 months ago

jardakotesovec commented 6 months ago

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:

  1. 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.

Screenshot 2024-03-27 at 16 54 55

PRs

OJS: pkp/ojs#4275 PKP-LIB: #9967 UI-LIBRARY: pkp/ui-library#349

New PRs (previous replaced) OJS: pkp/ojs#4281 OMP: pkp/omp#1576 OPS: pkp/ops#691 PKP-LIB: #9974 UI-LIBRARY: pkp/ui-library#354

jardakotesovec commented 6 months ago

@defstat Hi Dimitris, could you have a look? Thank you!

defstat commented 4 months ago

@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:

image

jardakotesovec commented 4 months ago

@defstat Nice to see the size reduction. I made some suggestions, let me know what you think.

defstat commented 4 months ago

@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.

jardakotesovec commented 4 months ago

@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 commented 4 months ago

@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.

jardakotesovec commented 4 months ago

@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.

defstat commented 4 months ago

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.

jardakotesovec commented 4 months ago

@defstat What about src/components/CodeHighlighter/CodeHighlighter.vue ? Just in case we want to support more options in future :-).

defstat commented 4 months ago

ok @jardakotesovec! I added the possibility of selecting from a set of languages and pushed the code. We can either

  1. leave those,
  2. add only XML or
  3. add the import hljs from 'highlight.js/lib/common'; instead of import hljs from 'highlight.js/lib/core';.

The more languages we support, the bigger the lib size it gets. I leave this decision on you.

jardakotesovec commented 4 months ago

@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.

defstat commented 4 months ago

@jardakotesovec thanks. I think I concluded the review changes required and added the appropriate files for the StoryBook.

Pushed everything for your approval.

defstat commented 4 months ago

@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

defstat commented 4 months ago

@jardakotesovec everything merged.

jardakotesovec commented 4 months ago

@defstat Excellent, thank you!