Syntax highlighting is a CPU-intensive process which we run a lot while rendering posts and while using the composer preview. Moving it to a background worker releases the main thread to the browser, which makes the UX much smoother.
This is very, very cool.
While we’re in here, could we pass the site settings through as a parameter? This is going to be a big focus of upcoming work and you might as well kill two birds with one stone.
Is there a way we can avoid attaching this onto
Discourse? That is causing us issues with the migration to Ember-CLI. You could store it in a variable in this lib with a method to set it?
Would attaching it to
Site.current() be ok? Happy to update some others at the same time. At a quick glance, we have these similar variables:
Site should probably be fine, although again I’d advice to pass the site in and not call
current() if possible. Still, as long as it’s not on
Discourse that works better.
As for the others I’d prefer to approach them on a case by case basis. It’s probable some would be better off elsewhere.
Trying to work out the cleanest way to design this. @eviltrout how do you feel about having an Ember Service for syntax highlighting?
Then rather than importing a function, consumers would
inject the highlighting service and call a method on it. That way, the service will have direct access to site settings and the singleton site via dependency injection.
That sounds quite nice! Let’s do it.
very cool change
I wonder how far we will end up taking this, much longer term we can even do markdown.it in a service worker.
I worked on moving this to an Ember service, but it ended up being a lot more complicated than before, and harder to use. So I switched back to a basic ES6 module.
@samsaffron yes I would love to move markdown.it as well. It is a lot more complicated than highlightjs, because it has lots of interdependencies with the app, but still totally achievable. If this is merged and runs successfully then I’d certainly like to spend some time trying to move markdown.
Overall I am pretty happy with where this is at right now. I think it is ready to merge.
@davidtaylorhq yes that seems quite reasonable!