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

Reduce highlights.js bundle size #9840

Closed
jardakotesovec opened this issue Mar 27, 2024 · 15 comments
Closed

Reduce highlights.js bundle size #9840

jardakotesovec opened this issue Mar 27, 2024 · 15 comments
Assignees
Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. UI/UX Issues affecting the user interface/user experience
Milestone

Comments

@jardakotesovec
Copy link
Contributor

jardakotesovec commented Mar 27, 2024

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 jardakotesovec added Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. UI/UX Issues affecting the user interface/user experience labels Mar 27, 2024
@jardakotesovec jardakotesovec added this to the 3.5 Internal milestone Mar 27, 2024
@jardakotesovec
Copy link
Contributor Author

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

defstat added a commit to defstat/ojs that referenced this issue May 18, 2024
defstat added a commit to defstat/pkp-lib that referenced this issue May 18, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 18, 2024
@defstat
Copy link
Contributor

defstat commented May 18, 2024

@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
Copy link
Contributor Author

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

defstat added a commit to defstat/ojs that referenced this issue May 21, 2024
defstat added a commit to defstat/pkp-lib that referenced this issue May 21, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 21, 2024
@defstat
Copy link
Contributor

defstat commented May 21, 2024

@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
Copy link
Contributor Author

jardakotesovec commented May 21, 2024

@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
Copy link
Contributor

defstat commented May 21, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor

defstat commented May 21, 2024

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
Copy link
Contributor Author

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

defstat added a commit to defstat/pkp-lib that referenced this issue May 21, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 21, 2024
@defstat
Copy link
Contributor

defstat commented May 21, 2024

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
Copy link
Contributor Author

jardakotesovec commented May 22, 2024

@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 added a commit to defstat/ui-library that referenced this issue May 23, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 23, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 23, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 23, 2024
@defstat
Copy link
Contributor

defstat commented May 23, 2024

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

Pushed everything for your approval.

defstat added a commit to defstat/pkp-lib that referenced this issue May 28, 2024
defstat added a commit to defstat/pkp-lib that referenced this issue May 28, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 28, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 28, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 28, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 28, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 28, 2024
defstat added a commit to defstat/ui-library that referenced this issue May 28, 2024
defstat added a commit to defstat/ojs that referenced this issue May 29, 2024
defstat added a commit to defstat/omp that referenced this issue May 29, 2024
defstat added a commit to defstat/omp that referenced this issue May 29, 2024
defstat added a commit to defstat/omp that referenced this issue May 29, 2024
defstat added a commit to defstat/ops that referenced this issue May 29, 2024
defstat added a commit to defstat/ops that referenced this issue May 29, 2024
@defstat
Copy link
Contributor

defstat commented May 29, 2024

@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 added a commit to defstat/pkp-lib that referenced this issue May 31, 2024
defstat added a commit to defstat/pkp-lib that referenced this issue May 31, 2024
defstat added a commit to defstat/ojs that referenced this issue May 31, 2024
defstat added a commit to defstat/omp that referenced this issue May 31, 2024
defstat added a commit to defstat/omp that referenced this issue May 31, 2024
defstat added a commit to defstat/omp that referenced this issue May 31, 2024
defstat added a commit to defstat/ops that referenced this issue May 31, 2024
defstat added a commit to defstat/ops that referenced this issue May 31, 2024
defstat added a commit to defstat/ojs that referenced this issue May 31, 2024
defstat added a commit that referenced this issue May 31, 2024
defstat added a commit that referenced this issue May 31, 2024
defstat added a commit to pkp/ops that referenced this issue May 31, 2024
 [OPS][main] #9840 Remove vue3-highlightjs and from highlight.js use only XML language
defstat added a commit to pkp/omp that referenced this issue May 31, 2024
[OMP][main] #9840 Remove vue3-highlightjs and from highlight.js use only XML language
defstat added a commit to pkp/ojs that referenced this issue May 31, 2024
[OJS][main] #9840 Remove vue3-highlightjs and add "highlightjs/vue-plugin". Use only XML language.
@defstat
Copy link
Contributor

defstat commented May 31, 2024

@jardakotesovec everything merged.

@defstat defstat closed this as completed May 31, 2024
@jardakotesovec
Copy link
Contributor Author

@defstat Excellent, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. UI/UX Issues affecting the user interface/user experience
Projects
None yet
Development

No branches or pull requests

2 participants