Skip to content

Conversation

@kabiroberai
Copy link
Contributor

Dependency injection allows one to use a custom highlight.min.js file instead of the one provided (eg. to add a custom language). Since the init parameters all have default values, this shouldn't be source-breaking.

@kabiroberai kabiroberai changed the title Add support for Swift 4.2, dependency injection, and highlight.js' ignore option Add support for Swift 4.2, dependency injection, and highlight.js' ignoreIllegals option Jun 20, 2018
@kabiroberai
Copy link
Contributor Author

There are also two other areas that could do with improvement, but I didn’t change those since I thought you might have written them the way you did for a reason:

  1. The language js files needn’t be included in the bundle — highlight.min.js already contains them
  2. Using the JavaScriptCore callMethod(_:withArguments:) API would probably be faster and less error-prone than calling jsContext.evaluateScript, and additionally you wouldn’t need to escape the code string manually.

Do let me know whether I should add these changes to my PR :)

@raspu
Copy link
Owner

raspu commented Jun 20, 2018

Hi @kabiroberai thank you for the PR!

Everything looks in order, but let me take some time to test it. Also now that you are introducing a Shims file, I will move the rest of the shimming code there.

@raspu
Copy link
Owner

raspu commented Jun 20, 2018

Your other suggestions sound great also, I never noticed that highlight.min.js also includes the rest of the language files, do you know if the CSS files are also included? (I am guessing they are not). If they aren't, I am not that sure if adding dependency injection is a good idea to be honest, unless the library is injected as a structured bundle.

@kabiroberai
Copy link
Contributor Author

As far as I can tell, the CSS files aren’t included in highlight.min.js. The primary use of dependency injection would be to add custom languages, as there's already a plethora of themes to choose from. That’s why the initialiser only allows you to customise the path to highlight.min.js (it always uses the framework bundle for themes)

@raspu
Copy link
Owner

raspu commented Jun 20, 2018

I just confirmed that they are not included in the minified JS, but I also confirmed that there's no reference to them in the code itself and that they are found at run-time by the library. So you are right, it seems it's safe to introduce dependency injection, we just need to make sure that the JS file can always find the embedded CSSs. I was worried that maybe the list of themes was hard coded into the library, but it seems the library is more sophisticated than I thought.

So if you are willing to include your proposed changes in this PR also, please go ahead, I will wait for you.

@kabiroberai
Copy link
Contributor Author

Done; the PR should be ready to merge now

@raspu raspu merged commit bd57404 into raspu:master Jul 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants