Replace vscode-oniguruma with oniguruma-to-es #55

remcohaszing · 2025-11-21T12:29:35Z

This replaces the dependency on the oniguruma wasm build. The code is a modified version of
https://github.com/shikijs/shiki/blob/main/packages/engine-javascript. The upstream has an option forgiving, but we don’t. We can add it or remove it. We may also be able to increase the scope in which the cache is used. It can be global or per starry-night instance.

The reason I tried this, was to decrease memory usage in a Next.js build. It didn’t work. We can still discuss whether or not this is an improvement.

Closes #50

This replaces the dependency on the oniguruma wasm build. The code is a modified version of https://github.com/shikijs/shiki/blob/main/packages/engine-javascript. The upstream has an option `forgiving`, but we don’t. We can add it or remove it. We may also be able to increase the scope in which the cache is used. It can be global or per starry-night instance.

remcohaszing · 2025-11-21T12:31:15Z

lib/onig-lib.js

+import {toRegExp} from 'oniguruma-to-es'
+
+/** @type {boolean} */
+const forgiving = true


This is an option in shiki. We can make it an option, or make it true or false.

This is the reason 100% coverage isn’t met and CI fails.

What are the tradeoffs?
I’d imagine the wasm currently is not forgiving, so changing that is also a break. Why?

ohh, this is more like “silent”/“swallow”? Then yes, I do think it is the default currently to swallow regex errors?

no no jsdoc type needed too

This is about swalling regex syntax errors in the grammars. I just tested the bahaviour on the main branch. The original behaviour is the same as having forgiving = false here.

TBH I believe grammar regexes should just be syntactically valid. IMO we should just throw instead of being invalid.

The jsdoc type is needed to make this a boolean instead of true. This prevents TypeScript from detecting and removing dead code. I will remove the variable altogether. I just need it while we decide on being forgiving or not.

remcohaszing · 2025-11-21T12:36:58Z

index.d.ts

@@ -1,4 +1,4 @@
-export type {GetOnigurumaUrl, Grammar, Options} from './lib/types.js'
+export type {Grammar} from './lib/types.js'


I wonder if the removal of these options should be considered a breaking change.

Yes, breaking. I think this is a major regardless, as this is supposed to work 99.99% correctly, but thus not the exact same

wooorm · 2025-11-22T17:34:25Z

index.d.ts

@@ -1,4 +1,4 @@
-export type {GetOnigurumaUrl, Grammar, Options} from './lib/types.js'
+export type {Grammar} from './lib/types.js'


Yes, breaking. I think this is a major regardless, as this is supposed to work 99.99% correctly, but thus not the exact same

wooorm · 2025-11-22T17:35:14Z

lib/index.js

        return grammar
      },
-      onigLib: createOniguruma(options)
+      onigLib: import('./onig-lib.js')


why dynamic?

onigLib must be a promise that resolves to this module. It’s a bit silly IMO. We can also import it statically and pass Promise.resolve(onigLib). This was just easy to write while the API isn’t final yet.

If we decide to take options, this will change anyway.

wooorm · 2025-11-22T17:35:55Z

lib/onig-lib.js

+/** @type {boolean} */
+const forgiving = true
+
+const MAX = 2 ** 32 - 1


i prefer lowercase, and a comment describing this magic value is a good idea

I agree. The number is taken from https://github.com/shikijs/shiki/blob/v3.15.0/packages/engine-javascript/src/scanner.ts#L7. I figured out it looked suspiciously like a power of 2 minus 1.

I don’t know why it has this value. Perhaps @antfu knows.

wooorm · 2025-11-22T17:36:49Z

lib/onig-lib.js

+import {toRegExp} from 'oniguruma-to-es'
+
+/** @type {boolean} */
+const forgiving = true


What are the tradeoffs?
I’d imagine the wasm currently is not forgiving, so changing that is also a break. Why?

wooorm · 2025-11-22T17:37:10Z

lib/onig-lib.js

+      return pattern
+    }
+
+    // Cache


useless comment?

I preserved comments from where I got the code. I do agree this comment is useless. I can remove it.

wooorm · 2025-11-22T17:45:55Z

lib/onig-lib.js

+      /** @type {Array<[index: number, match: RegExpExecArray]>} */
+      const pending = []
+
+      for (const [index, regexp] of regexps.entries()) {


i prefer calling these expression(s)

wooorm · 2025-11-22T17:47:56Z

lib/onig-lib.js

+          if (match.index === minIndex) {
+            return toResult(index, match)
+          }
+        }


these 2 loops can be made into one: iterate over each and find the object whose index is the lowest. When done, return that found object?

wooorm · 2025-11-22T17:49:33Z

lib/onig-lib.js

+}
+
+/**
+ * Do nothing


Can you use .s to end such whole sentences in comments (generally, if the first word is also capitalized, and it could be multiple sentences, I use dots)?

wooorm · 2025-11-22T17:52:36Z

lib/onig-lib.js

+          end: indice[1],
+          length: indice[1] - indice[0]
+        }
+      }) || []


can you sort the fields of these 3 objects

seems weird to use this max value, is it the last index into some source string? Could that value be passed in as a parameter?

Sort how?

Re max value: see review comment on its definition.

Re passing in as a parameter: You mean to always call as toResult(…, …, max)? I don’t really see the point TBH.

The original code also has an offset, but it’s always 0.

wooorm · 2025-11-22T17:54:34Z

lib/onig-lib.js

+ * @param {RegExpExecArray} match
+ *   The actual match
+ * @returns {IOnigMatch}
+ *   The oniguruma match.


Seems useless to me to prefix every sentence with The; I’d do: Index in source value; Regex match; Oniguruma match or so?

remcohaszing · 2025-11-23T10:19:40Z

lib/onig-lib.js

+import {toRegExp} from 'oniguruma-to-es'
+
+/** @type {boolean} */
+const forgiving = true


This is about swalling regex syntax errors in the grammars. I just tested the bahaviour on the main branch. The original behaviour is the same as having forgiving = false here.

TBH I believe grammar regexes should just be syntactically valid. IMO we should just throw instead of being invalid.

The jsdoc type is needed to make this a boolean instead of true. This prevents TypeScript from detecting and removing dead code. I will remove the variable altogether. I just need it while we decide on being forgiving or not.

remcohaszing · 2025-11-23T10:25:41Z

lib/onig-lib.js

+/** @type {boolean} */
+const forgiving = true
+
+const MAX = 2 ** 32 - 1


I agree. The number is taken from https://github.com/shikijs/shiki/blob/v3.15.0/packages/engine-javascript/src/scanner.ts#L7. I figured out it looked suspiciously like a power of 2 minus 1.

I don’t know why it has this value. Perhaps @antfu knows.

remcohaszing · 2025-11-23T10:27:18Z

lib/onig-lib.js

+      return pattern
+    }
+
+    // Cache


I preserved comments from where I got the code. I do agree this comment is useless. I can remove it.

remcohaszing · 2025-11-23T10:31:01Z

lib/onig-lib.js

+          // change in meaning since TM grammars search line by line
+          singleline: true
+        }
+      })


I have no strong opinions on this. We can make them configurable via options.

remcohaszing · 2025-11-23T10:32:47Z

lib/onig-lib.js

+      })
+      cache.set(pattern, regex)
+      return regex
+    } catch (error_) {


Me too, but XO insisted I call this error_.

If we decide to not be forgiving, there‘s no point in caching errors IMO. In that case I will remove the entire try/catch.

remcohaszing · 2025-11-23T10:38:51Z

lib/onig-lib.js

+          end: indice[1],
+          length: indice[1] - indice[0]
+        }
+      }) || []


Sort how?

Re max value: see review comment on its definition.

Re passing in as a parameter: You mean to always call as toResult(…, …, max)? I don’t really see the point TBH.

The original code also has an offset, but it’s always 0.

remcohaszing · 2025-11-23T10:39:34Z

lib/onig-lib.js

+      }
+
+      if (forgiving) {
+        return null


I also prefer undefined, but the API expects null.

remcohaszing · 2025-11-23T10:42:21Z

lib/index.js

        return grammar
      },
-      onigLib: createOniguruma(options)
+      onigLib: import('./onig-lib.js')


onigLib must be a promise that resolves to this module. It’s a bit silly IMO. We can also import it statically and pass Promise.resolve(onigLib). This was just easy to write while the API isn’t final yet.

If we decide to take options, this will change anyway.

remcohaszing · 2025-11-23T10:44:42Z

lib/onig-lib.js

+ */
+export function createOnigScanner(sources) {
+  /** @type {Map<string, RegExp | Error>} */
+  const cache = new Map()


At what level do we want to have this cache?

remcohaszing commented Nov 21, 2025

View reviewed changes

Fix linting issues

28d3536

wooorm reviewed Nov 22, 2025

View reviewed changes

remcohaszing commented Nov 23, 2025

View reviewed changes

		@@ -1,4 +1,4 @@
		export type {GetOnigurumaUrl, Grammar, Options} from './lib/types.js'
		export type {Grammar} from './lib/types.js'

Uh oh!

Replace vscode-oniguruma with oniguruma-to-es #55

Are you sure you want to change the base?

Replace vscode-oniguruma with oniguruma-to-es #55

Uh oh!

Conversation

remcohaszing commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants