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

[Enhancement request] Allow importing named language imports #3470

Closed
webketje opened this issue Jan 28, 2022 · 9 comments
Closed

[Enhancement request] Allow importing named language imports #3470

webketje opened this issue Jan 28, 2022 · 9 comments
Labels
enhancement An enhancement or new feature parser

Comments

@webketje
Copy link

Is your request related to a specific problem you're having?
When using a bundler on the front-end it is important to only include the required languages to reduce bundle size,
but it feels a bit clunky to register languages like this:

import hljs from 'highlight.js/lib/core';
import js from 'highlight.js/lib/languages/javascript';
import json from 'highlight.js/lib/languages/json';
import yml from 'highlight.js/lib/languages/yaml';
import md from 'highlight.js/lib/languages/markdown';

hljs.registerLanguage('javascript', js);
hljs.registerLanguage('json', json);
hljs.registerLanguage('yaml', yml);
hljs.registerLanguage('markdown', md);

The solution you'd prefer / feature you'd like to see added...
It would be much better if it could just be done like this:

import hljs from 'highlight.js/lib/core';
import { javascript, json, yaml, markdown } from 'highlight.js/lib/languages';

hljs.registerLanguage('javascript', javascript);
hljs.registerLanguage('json', json);
hljs.registerLanguage('yaml', yaml);
hljs.registerLanguage('markdown', markdown);

This only requires adding lib/languages/index.js which has lines like:

export { default as json } from './json';
export { default as javascript } from './javascript';
// aliases could be done like this:
export { default as js } from './javascript';

I could make a PR for this (please mention whether aliasing is desirable, I believe this could also be implemented in client code as import { javascript as js } from 'highlight.js/languages')

@webketje webketje added enhancement An enhancement or new feature parser labels Jan 28, 2022
@joshgoebel
Copy link
Member

joshgoebel commented Jan 29, 2022

Aren't dynamic imports the true solution to this problem?

please mention whether aliasing is desirable

It's definitely not as aliases aren't even guaranteed to be unique (not even in core, sadly).

@webketje
Copy link
Author

webketje commented Jan 29, 2022

Ok for aliases, but..

Aren't dynamic imports the true solution to this problem?

..misses the point (the issue is about DX). I'll try to rephrase: requiring a user (developer) of highlight.js to write 10 separate import lines is way less developer-friendly then allowing them to write a single import line with named imports, like import {lang1, lang2, lang3, lang4 } from 'highlight.js/languages'. Think it's pretty standard practice for developer convenience to add an index.js that exports all members of a dir as named exports

Static vs dynamic imports will work with both.

@joshgoebel
Copy link
Member

joshgoebel commented Jan 30, 2022

the issue is about DX)

Exactly, when I said dynamic I was imagining of something like:

const supportedLanguages = ["basic", "cobol", "other"]
supportedLanguages.forEach(lang => {
  import(`${languages_dir}/${lang}`).then(hljs.registerLanguage)
});

No need to repeat anything.

@webketje
Copy link
Author

webketje commented Jan 30, 2022

Aha ok, but that makes the import async (and may trigger unintentional bundle splitting in tools like webpack. In webpack this would generate x chunks where x=language. It doesn't read as nicely and doesn't allow import type hints/clickthrough (VS code)

@joshgoebel
Copy link
Member

joshgoebel commented Jan 30, 2022

I'm always cautious with new feature requests because often the full expense is rarely counted (many times it's impossible to know and paid later). For example this small request leads to:

  • updating the ES6 node build code
  • updating the CJS node build code (I assume we'd want parity)
  • or would we say this is for the ES6 people only, and would that decision itself cause any problems?
  • updating the ES6 CDN build
  • updating the CJS CDN build
  • ...and of course (for now) these may need to be ES6 -> CJS pointer scripts (to avoid people requiring & importing two difference instances via outside libraries)
  • Which means when we finally drop CJS support we may need to revise this again to generate pure ES6 indexes
  • the README will need to be updated with new examples/docs
  • Do we then deprecate [remove from docs] the old way? I don't think we can because 3rd party grammars would still do their imports this way because they won't be part of the core bundle index.
  • So it seems like now we have to document both approaches?

And that's just only off the top of my head with only a minutes thought. :) So I ask myself is all that (and potential unknowns) worth it for a nice idea (truly) that no one else has ever brought to our attention before. I wonder if this wouldn't be worth re-considering more after we drop CJS support in the future, when our build pipeline theoretically becomes much simpler.

I could make a PR for this

Were you imagining tackling all that? Thoughts?

@webketje
Copy link
Author

webketje commented Jan 31, 2022

@joshgoebel Thx for the detailed analysis, I need to have a go at it. I assumed the ES6/CJS/CDN compatibility was handled by a build tool from a single source, but it can also be done manually without too much effort. It would definitely be useful for CJS too: const { markdown, javascript } = require('highlight.js/lib/languages').

Do we then deprecate [remove from docs] the old way?
So it seems like now we have to document both approaches?

I would say yes, hide in the docs (but keeping the ability in the code)

I don't think we can because 3rd party grammars would still do their imports this way because they won't be part of the core bundle index.

I think that's not an issue:

import { lang1, lang2, lang3 } from 'highlight.js/lib/languages';
import thirdPartyLang from 'thirdpartyLang';

when we finally drop CJS support we may need to revise this again to generate pure ES6 indexes

This & some of the other questions I can only know after getting more familiar with the dev setup & build of highlight.js
So let me have a stab at it, and if you like what you see, you can decide whether it's worth it, and with what kind of version change to ship it.

I did imagine having to create docs, unit tests etc as I maintain the metalsmith SSG (whose website metalsmith.io uses highlight.js)

@joshgoebel
Copy link
Member

I assumed the ES6/CJS/CDN compatibility was handled by a build tool from a single source

There is a single set of canonical source files, yes - I was only saying the build scripts would have to be updated to generate the necessary files, etc... I didn't know if you were considering ES6 + CJS and weren't sure if you were aware we have multiple deliverable assets (CDN, node, browser, etc)... browser isn't relevant here as that's just a monolith.

So let me have a stab at it, and if you like what you see, you can decide whether it's worth it, and with what kind of version change to ship it.

Sure if you want to look into it with said caveat, go right ahead.

@joshgoebel
Copy link
Member

joshgoebel commented Feb 15, 2022

▲ = node ./build/es/test-es-imports-index
The script uses approximately 7.31 MB
▲ = node ./build/es/test-es-imports
The script uses approximately 3.83 MB
▲ = node ./build/lib/test-cjs-imports-index
The script uses approximately 7 MB
▲ = node ./build/lib/test-cjs-imports
The script uses approximately 3.52 MB

So correct, it adds about 3.5MB of RAM for all languages. I'd argue that's negligible for Node

I think this line of thinking is why modern software can be so slow when computers are faster than they'd ever been... I just don't see a large enough win here to justify these changes. We increase code complexity, compile times, increase RAM usage...

it would be dramatic (and unintended) if it were bundled in web bundlers though

...and potentially vastly increase the download size if someone's bundler isn't properly tree pruning or analyzing side effects properly... (a bundler or config problem)

Too many negatives for a tiny quality of life change that truly only effects someone just getting started with Highlight.js on a new project. Just adding a new language to an existing file takes almost no time and the list of imports isn't something someone one needs to constantly fiddle with...


I did add this to the v12 list to re-review to see if anything has changed.

Closing this issue and the PR for now.

@joshgoebel
Copy link
Member

In reviewing this again (just now) for consideration for v12 I still find it lacking in that it would potentially force a ~ megabyte of unneeded JS to be downloaded and parsed. IMHO this makes more sense if/when we go fully async so that registering the languages doesn't actually perform any actual requests or parsing... so one could either say:

  • register these languages, then allow them to later load on demand

OR

  • register these languages, and pre-load them now

But in both cases only the specified languages would ever be download and parsed, not every single language that was in the index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature parser
Projects
None yet
Development

No branches or pull requests

2 participants