cactbot
cactbot copied to clipboard
build: add suo-parser for webpack load timeline
Import suo-parser for parsing timeline.
suo-parser
is written by me about half a year ago, but I didn't have time until now to clean up the code and publish it to npm.
Unsurprisingly, suo-parser
supports cactbot
style timeline syntax only now (except deprecated infotext
, alerttext
and alarmtext
).
It will accelerate the build step for about 5 sec in my test.
Additionally, I have thought about how the raidboss module loads timeline in runtime:
Currently, raidboss reads and processes the timeline when the player enters a new zone and some timelines match the zone id. But because we use webpack to pack them into a bundle, the timeline won't be changed in runtime, so why don't we just provide a processed timeline object in the bundle? Additionally, we can make this object uglified/simplified, decreasing the release size but didn't affect the function. However, the processing code that reads timeline should be still in the release, we'll use it to process custom timeline text that the user provides.
A couple of things:
- If this is faster (and cleaner? It looks like it uses an AST and not just a bunch of imperative regex and conditionals), then should all timeline parsing in general be replaced by this rather than just the webpack end of it?
- Along those lines, I am a little bit hesitant to add an external dependency for timeline, because then it would make updating timelines harder (if we ever added extra keywords)? This isn't a blocker, as you could update suo-parser, and then update the dependency, and then update cactbot, but I think it would be extra friction and would probably catch somebody by surprise.
- If this is faster (and cleaner? It looks like it uses an AST and not just a bunch of imperative regex and conditionals), then should all timeline parsing in general be replaced by this rather than just the webpack end of it?
Yeah, that's what I wanted to do. But I think there are more work before apply it to timeline parsing in general, for example, we can easily force the format of timeline in this repo, there are basically no error when parsing timelines in the repo, but parsing timelines from user needs more error catching or something else.
- Along those lines, I am a little bit hesitant to add an external dependency for timeline, because then it would make updating timelines harder (if we ever added extra keywords)? This isn't a blocker, as you could update suo-parser, and then update the dependency, and then update cactbot, but I think it would be extra friction and would probably catch somebody by surprise.
If you are going to create an organisation for cactbot (just like they XivLauncher or xivapi do), I don't mind to transfer this parser to the organisation that you can easily update.
- If this is faster (and cleaner? It looks like it uses an AST and not just a bunch of imperative regex and conditionals), then should all timeline parsing in general be replaced by this rather than just the webpack end of it?
Yeah, that's what I wanted to do. But I think there are more work before apply it to timeline parsing in general, for example, we can easily force the format of timeline in this repo, there are basically no error when parsing timelines in the repo, but parsing timelines from user needs more error catching or something else.
Yeah, I would rather have a single timeline parser than have two timeline parsers. I think it would be very confusing to have two separate ones (one in timeline.ts and the other in suo-parser) that (potentially) behave differently or parse things in different ways. Once that work is done to add error catching or whatever else is missing, then maybe it can be added to replace both loading and timeline.ts and we can still have only a single timeline parser?
- Along those lines, I am a little bit hesitant to add an external dependency for timeline, because then it would make updating timelines harder (if we ever added extra keywords)? This isn't a blocker, as you could update suo-parser, and then update the dependency, and then update cactbot, but I think it would be extra friction and would probably catch somebody by surprise.
If you are going to create an organisation for cactbot (just like they XivLauncher or xivapi do), I don't mind to transfer this parser to the organisation that you can easily update.
Ah, it's less about who owns it, and more about the "outside the repository" thing. It makes updates require three commits when you have two separate git repositories. As I said, it's not a blocker for me (so long as the licensing is such that I could include it in cactbot directly if you ever disappeared).
Well, I think I can implement an interface for grammar extension, just like what babel or eslint does.
In my opinion, I would rather prefer to split it as a dependency, as npm supports installing dependencies from a git repo or even local directory (which compatiable with git-submodules), I think it is better that one package does one thing? And I think a modification for timeline grammar/syntax should not be very frequent, so it might not very painful to have three commits to do an update?
we have moved timeline loader to runtime and remove timeline-loader.ts
, should we close this PR?
we have moved timeline loader to runtime and remove
timeline-loader.ts
, should we close this PR?
OK