pkp-lib icon indicating copy to clipboard operation
pkp-lib copied to clipboard

Reduce highlights.js bundle size

Open jardakotesovec opened this issue 10 months ago • 1 comments

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

jardakotesovec avatar Mar 27 '24 15:03 jardakotesovec

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

jardakotesovec avatar Mar 27 '24 16:03 jardakotesovec

@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

defstat avatar May 18 '24 09:05 defstat

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

jardakotesovec avatar May 21 '24 10:05 jardakotesovec

@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 avatar May 21 '24 12:05 defstat

@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 avatar May 21 '24 13:05 jardakotesovec

@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 avatar May 21 '24 13:05 defstat

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

jardakotesovec avatar May 21 '24 14:05 jardakotesovec

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 avatar May 21 '24 14:05 defstat

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

jardakotesovec avatar May 21 '24 14:05 jardakotesovec

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.

defstat avatar May 21 '24 15:05 defstat

@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 avatar May 22 '24 07:05 jardakotesovec

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

Pushed everything for your approval.

defstat avatar May 23 '24 11:05 defstat

@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 avatar May 29 '24 10:05 defstat

@jardakotesovec everything merged.

defstat avatar May 31 '24 09:05 defstat

@defstat Excellent, thank you!

jardakotesovec avatar Jun 01 '24 08:06 jardakotesovec