Remove use of global highlight instance #51
Conversation
test/index.js
Outdated
| import {rehype} from 'rehype' | ||
| import {removePosition} from 'unist-util-remove-position' | ||
| import {lowlight} from '../index.js' | ||
| import {lowlight} from '../lib/all.js' |
There was a problem hiding this comment.
Why mock? Wouldn’t it be better to actually test whether the two interfere?
There was a problem hiding this comment.
As the configuration can't be read, we would have to compare the results of highlight and lowlight. Would you prefer that instead of checking if configure is called?
There was a problem hiding this comment.
Btw. I had to switch to ../lib/all.js, as not all languages of the fixture folder are covered by the common setup. Would you rather prefer to remove the fixtures?
And another topic - locally I saw this failing check, but the message wasn't clear to me 😬
/home/runner/work/lowlight/lowlight/lib/all.d.ts:2:13: DoNotTochItRegistersLanguageFiles
1846 / 1847 99.94%
The type coverage rate(99.94%) is lower than the target(100%).
There was a problem hiding this comment.
To get the tests working, I think this is the trick:
import highlight from 'highlight.js'
+import coffeescript from 'highlight.js/lib/languages/coffeescript'
+import haskell from 'highlight.js/lib/languages/haskell'
+import http from 'highlight.js/lib/languages/http'
+import pgsql from 'highlight.js/lib/languages/pgsql'+lowlight.registerLanguage('coffee', coffeescript)
+lowlight.registerLanguage('haskell', haskell)
+lowlight.registerLanguage('http', http)
+lowlight.registerLanguage('pgsql', pgsql)I also think you’d also need:
- assert.deepEqual(
+ assert.notDeepEqual(
lowlight.listLanguages(),
expectedLanguages,
- 'should return the same list of languages as highlight.js'
+ 'should *not* return the same list of languages as highlight.js'
)
The tests failing for this actually shows that everything works, so that’s great :)
There was a problem hiding this comment.
With this, I actually think it’s fine to not add highlight.js tests. Or mocking: so these changes you made to the tests.
But I am fine with an actual test, that checks that if a language is registered with lowlight, it still crashes when trying to use it with regular highlight.js?
There was a problem hiding this comment.
And at this point there's no need to stick with the current hljs-like global API.
Curious why you felt that was a requirement before?
There was a problem hiding this comment.
Meaning I need to think about how this should look.
And I'm AFK a bit these weeks!
Thanks for the response @wooorm. Let me know if I can help out somehow ✌️
There was a problem hiding this comment.
@joshgoebel because this was created years ago, when hljs supported much less, no emitters for example. And global. So it was a subclass. Now that can change. Perhaps a similar API to https://github.com/wooorm/starry-night. The flagToScope part particularly might be nice to have, which is a bit involved, I’d need some mapping between GH linguist and HLJS grammars
There was a problem hiding this comment.
Meaning I need to think about how this should look. And I'm AFK a bit these weeks!
@wooorm did you already have time to think about the options? Let me know if I can support somehow ✌️
There was a problem hiding this comment.
Haven’t found the time yet, I’m busy with major updates through the ±500 projects maintained in the unified collective.
That being said, in a couple days I’ll reach https://github.com/rehypejs/rehype-highlight, which uses this, at which point I’d like to have this change in it. So I’ll come up with something.
dschwank
force-pushed
the
fix/use-highlight-instance
branch
from
bfaa9cb
to
5b9d947
Compare
wooorm
changed the title
|
released in |
|
@wooorm Thanks a lot for merging and creating a new release! |
With this PR we make use of the
newInstancefunction ofhighlight.js, to not pollute the global instance anymore. As the configuration is local only now, we don't have to reset it anymore.Closes #47