Is it ok to reuse Prism object when loading multiple languages? #1393

CoenraadS · 2018-04-15T18:00:53Z

Hey

I noticed that if I do:

loadLanguages(["javascript"]);

and then later loadLanguages(["markup", "javascript"]);

Then the second one won't parse the javascript. However if ["markup", "javascript"] is loaded first it works correctly.

The text was updated successfully, but these errors were encountered:

Golmote · 2018-04-15T18:30:39Z

Hi, can you provide a full runnable snippet to illustrate the issue please? I'm not sure I get it.

CoenraadS · 2018-04-15T18:46:52Z

var Prism = require("prismjs/components/prism-core.js");
var loadLanguages = require("prismjs/components/index.js");

loadLanguages("javascript"); // This line causes script parsing to break in the following html file
loadLanguages(["markup", "javascript"]);

var someFile = 
`
<!DOCTYPE html>
<html lang="en"> 
  <head>
    <script>
     export default 
     {
       v
     }
    </script>
</html>
`

var tokenized = Prism.tokenize(someFile, Prism.languages["markup"]);
console.log(tokenized);

Golmote · 2018-04-15T18:52:57Z

Oh, OK I see now. The JS components adds support for JS in Markup only if the Markup component is loaded before the JS component is. Hence, when you first load the JS component, the Markup component is not loaded and the support is not added. When you then load Markup + JS, it too late because of require()'s cache... and the JS component won't be executed again.
We should find a way to invalidate the cache here.

mAAdhaTTah · 2018-04-15T19:07:18Z

Basic idea is this:

delete require.cache[require.resolve(pathToLanguage)]

Would make a Good First Issue™.

Golmote · 2018-04-16T23:03:51Z

Should we do this on every call? Or should we provide a second parameter to force the cache invalidation?
First one sounds quite inefficient. But the second one requires some knowledge of the inner behaviour of the components...

CoenraadS · 2018-04-17T05:56:33Z

If its done that way, will previously loaded languages still work?

Golmote · 2018-04-17T06:15:08Z

Yes, they should. I don't think reloading a component can break any previously loaded component.

mAAdhaTTah · 2018-04-17T11:54:42Z

I don't think it's that inefficient. We're not loading that many files. Users also probably shouldn't be loading the same language multiple times anyway. Maybe document all of this and just invalidate every time?

Adding a second parameter seems like the type of thing people will just throw in when things aren't working.

Golmote · 2018-04-17T19:17:42Z

Alright, agreed.

Golmote · 2018-04-22T13:43:27Z

I don't think reloading a component can break any previously loaded component.

Actually, I am wrong about this.

Consider we do implement the cache clearing, and a user wants to highlight JS in HTML:

This will work, obviously:

loadLanguages(['markup', 'javascript']);

This will work too, thanks to the cache being cleared:

loadLanguages('javascript'); loadLanguages(['markup', 'javascript']);

But this will break, because of the cache being cleared...

loadLanguages(['markup', 'javascript']); loadLanguages('markup');

Slightly more subtle, this won't work either, because markdown requires markup and thus refreshes it:

loadLanguages(['markup', 'javascript']); loadLanguages('markdown');

I'm really reconsidering the idea of the second parameter...

mAAdhaTTah · 2018-04-22T16:27:11Z

Wow, nice catch.

If we wanted to still avoid said second parameter, we'd have to resolve them in the loadLanguages function, e.g. when we do this:

loadLanguages(['markup', 'javascript']); loadLanguages('markup');

We'd have to reload javascript after we've loaded markup the second time.

Is that "optional dependency" made explicit anywhere? We could add optional dependencies to components.js, and reload them conditionally based on whether they've been loaded already or not.

It's not ideal, from an implementation perspective, but I'd much rather pull that complexity into Prism rather than expecting users to understand whether or not they should be invalidating the cache. Understanding whether to do so requires users to understand Prism's internals, which I'd like to avoid even at the expense of making loadLanguages more complex.

Golmote · 2018-05-01T14:49:02Z

Ok, I dug into this.

Here is the list of the "optional dependencies" I've come up with:

Language	Optional dependencies
CSS	Markup
JavaScript	Markup
ActionScript	Markup
Django	CSS, JavaScript
Haml	CSS, CoffeeScript, ERB, JavaScript, Less, Markdown, Ruby, SCSS, Textile
HTTP	JavaScript, Markup
OpenCL	C, C++ (***)
Pug	CoffeeScript, EJS, Handlebars, (Hogan), Less, LiveScript, Markdown, (Mustache), (Plates), SCSS, Stylus, (Swig)
Pure	C, C++, Fortran, (ATS), (DSP)
Textile	CSS

(*** This is a weird one: OpenCL extends C++ which extends C, but it also add keywords to both languages. Apparently this was done so that C/C++ code blocks could use the OpenCL host API and it would be properly highlighted. The thing that worries me the most here is the name optionalDependencies... since they are not optional at all in that specific case.)

As you can see, this is not such a long list. Yet, it may have huge impacts on the loading order.

One of the worst case scenario I've found is:

loadLanguages(['actionscript', 'http', 'textile', 'django', 'haml', 'css', 'javascript', 'markup', 'xeora']);

which will lead to the following files being actually required:

Click to expand the "graph".

c-like
- javascript
  - actionscript
http
markup
- -> reload dependents
  - javascript
  - actionscript
  - http
- textile
markup
- -> reload dependents
  - javascript
  - actionscript
  - http
- django
c-like
- ruby
  - haml
css
- -> reload dependents
  - django
  - haml
  - textile
    - -> reload dependents
      - haml
c-like
- javascript
  - -> reload dependents
    - django
    - haml
    - http
markup
- -> reload dependents
  - css
    - -> reload dependents
      - django
      - haml
      - textile
        
        -> reload dependents
        
        haml
  - javascript
    - -> reload dependents
      - django
      - haml
  - actionscript
  - http
markup
- -> reload dependents
  - css
    - -> reload dependents
      - django
      - haml
      - textile
        
        -> reload dependents
        
        haml
  - javascript
    - -> reload dependents
      - django
      - haml
  - actionscript
  - http
- xeora

(That's 50 files require'd, for 9 languages)

Obviously, calling loadLanguages() without paremeters also leads to a huge amounts of dependencies. (413 files required, for our 148 languages)

Am I overthinking an edge case?
Do you have alternative names in mind, for the OpenCL case?

mAAdhaTTah · 2018-05-01T15:38:52Z

I don't know if that's a big deal for node applications. A standard node_modules is pretty huge and has a ton of files, you're often installing a couple hundred dependencies and require'ing a boatload of files on startup.

Are people going to be re-require'ing language definitions often enough for this to be a problem? I'm inclined to think the surprise of it not working as expected and needing to dig into why would be worse than tracking down why so many files are loaded. That's also an easier problem to solve: load up all the required languages up front, then you don't get slowdown while your app is running.

Edit: That said, I'm not writing a lot of node applications, so I don't have a lot of experience here to lean on.

CoenraadS · 2018-05-01T16:56:24Z

Loading all languages is ~300ms for me (on demand loading was ~4ms). I can live with this since its a one time cost if everything else works correctly. If I'm the only one encountering this issue then you guys don't need to follow this up any further.

Previously I was calling loadLanguages('language') for every document the user opened, so it would be worse for my current situation if that got slower. One time slow or multiple times fast, but not multiple times slow 👍

Maybe when the node global issue is fixed, you can just keep a dictionary of languages and prism objects, and not worry about them conflicting since each object is independent.

mAAdhaTTah added the good first issue label Apr 15, 2018

Golmote added bug and removed good first issue labels Apr 23, 2018

Golmote self-assigned this Apr 28, 2018

Golmote mentioned this issue May 4, 2018

Handle optional dependencies in loadLanguages() #1417

Merged

Golmote mentioned this issue May 13, 2018

loadLanguages utility causes bloat with webpack #1403

Closed

Golmote closed this as completed in #1417 May 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Is it ok to reuse Prism object when loading multiple languages? #1393

Is it ok to reuse Prism object when loading multiple languages? #1393

CoenraadS commented Apr 15, 2018

Golmote commented Apr 15, 2018

CoenraadS commented Apr 15, 2018

Golmote commented Apr 15, 2018

mAAdhaTTah commented Apr 15, 2018

Golmote commented Apr 16, 2018

CoenraadS commented Apr 17, 2018

Golmote commented Apr 17, 2018

mAAdhaTTah commented Apr 17, 2018

Golmote commented Apr 17, 2018

Golmote commented Apr 22, 2018

mAAdhaTTah commented Apr 22, 2018

Golmote commented May 1, 2018

mAAdhaTTah commented May 1, 2018 •

edited

Loading

CoenraadS commented May 1, 2018 •

edited

Loading

Is it ok to reuse Prism object when loading multiple languages? #1393

Is it ok to reuse Prism object when loading multiple languages? #1393

Comments

CoenraadS commented Apr 15, 2018

Golmote commented Apr 15, 2018

CoenraadS commented Apr 15, 2018

Golmote commented Apr 15, 2018

mAAdhaTTah commented Apr 15, 2018

Golmote commented Apr 16, 2018

CoenraadS commented Apr 17, 2018

Golmote commented Apr 17, 2018

mAAdhaTTah commented Apr 17, 2018

Golmote commented Apr 17, 2018

Golmote commented Apr 22, 2018

mAAdhaTTah commented Apr 22, 2018

Golmote commented May 1, 2018

mAAdhaTTah commented May 1, 2018 • edited Loading

CoenraadS commented May 1, 2018 • edited Loading

mAAdhaTTah commented May 1, 2018 •

edited

Loading

CoenraadS commented May 1, 2018 •

edited

Loading