Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace JsonSchema with ajv for dictionary validation #304

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

djahandarie
Copy link
Collaborator

Replace JsonSchema with ajv for dictionary validation, as it is way faster.

In particular, this drops validation time of Jitendex from "10 to 40 minutes" to a few seconds.

It doesn't run into the potential eval issues with ajv, as this implementation compiles the schema in the build process and distributes them with the extension, as suggested by ajv's own documentation.

Some awkward edges:

  • Had to work around a bug in ajv which required vendoring in ucs2length from ajv.
  • Had to use some ugly dynamic import()s due to our codebase not currently using ECMAScript modules
  • Could not fully strip JsonSchema out from the codebase because other areas like the profile condition validation do actually dynamically generate templates, so there is no way to do that at build-time, and we cannot use ajv for those cases. There is no performance issue in those cases thankfully, but it is ugly to have two JSON schema validators.

@praschke @stephenmk PTAL 🙏

@djahandarie djahandarie requested a review from a team as a code owner November 3, 2023 14:48
themoeway-bot
themoeway-bot previously approved these changes Nov 3, 2023
Copy link

github-actions bot commented Nov 3, 2023

⚠️ Visual differences introduced by this PR; please validate if they are desirable.

View Playwright Report (note: open the "playwright-report" artifact)

@djahandarie
Copy link
Collaborator Author

djahandarie commented Nov 3, 2023

Looks like CI tests are failing because our tests set up a Node VM, and it would need to implement custom support for doing dynamic import()s by defining importModuleDynamically in dev/vm.js, but that might be a little annoying to write.

So instead, I'm trying to think if there is a way to do this without a dynamic import. Maybe we could write a wrapper/shim module which we load with <script type="module"> that grabs all the exports in the ajv-generated module and attaches them to the window object so they are accessible from non-module JS. 🤔

@stephenmk
Copy link

stephenmk commented Nov 4, 2023

@stephenmk PTAL 🙏

Incredible work! I built the code from the ajv branch and gave it a try. Seems to be working great.

I've been working on some new features for Jitendex (namely furigana for example sentences and extra entries from the proper noun dictionary) and I was worried about what kind of impact they might have on this validation performance. This update to yomitan would definitely dispel those worries.

@github-merge-queue github-merge-queue bot merged commit 3761510 into master Nov 9, 2023
4 of 6 checks passed
@djahandarie djahandarie added the kind/enhancement The issue or PR is a new feature or request label Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement The issue or PR is a new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants