search

OptimumCode / json-schema-validator

The JSON schema validation library that works with https://github.com/Kotlin/kotlinx.serialization
MIT License
33 stars 3 forks source link

Support `wasm` target #178

Closed mikepenz closed 2 months ago

mikepenz commented 2 months ago

Note

mikepenz commented 2 months ago

~Please don't merge yet. I want to quickly verify in a wasm project that it would work correctly.~

mikepenz commented 2 months ago

Ok verified that it works 👍

mikepenz commented 2 months ago

Missed the binary compatibility *.api file update (pushed the update)

Also noteworthy. the isNormalized was changed to be a top-level function instead of a function within an object.

I can change it back to an object, however, making an object expect is currently a beta function: EXPECT_ACTUAL_CLASSIFIERS_ARE_IN_BETA_WARNING which I am not sure you want.

Making the function within the object expect does not work.

mikepenz commented 2 months ago

Would it make sense to archive the artifacts for the checks: https://github.com/OptimumCode/json-schema-validator/actions/runs/10248584925/job/28350188962?pr=178#step:8:438 ?

Right now I believe they are not archived as a result.

OptimumCode commented 2 months ago

Would it make sense to archive the artifacts for the checks: https://github.com/OptimumCode/json-schema-validator/actions/runs/10248584925/job/28350188962?pr=178#step:8:438 ?

Right now I believe they are not archived as a result.

There should be comments from the review-dog that point out the problems but they were not published (probably because the token used by the utility does not have enough permissions when PR is created from the fork...). Please execute ./gradlew ktlintFormat task to fix format issues in the project.

OptimumCode commented 2 months ago

the isNormalized was changed to be a top-level function instead of a function within an object.

Don't have any objections to that)

mikepenz commented 2 months ago

Would it make sense to archive the artifacts for the checks: https://github.com/OptimumCode/json-schema-validator/actions/runs/10248584925/job/28350188962?pr=178#step:8:438 ? Right now I believe they are not archived as a result.

There should be comments from the review-dog that point out the problems but they were not published (probably because the token used by the utility does not have enough permissions when PR is created from the fork...). Please execute ./gradlew ktlintFormat task to fix format issues in the project.

I used ./gradlew ktlintFormat however it does not result in changes. (also the same command as on CLI doesn't seem to fail locally for me, which is strange)

mikepenz commented 2 months ago

Outstanding items I'll look into:

OptimumCode commented 2 months ago

I used ./gradlew ktlintFormat however it does not result in changes. (also the same command as on CLI doesn't seem to fail locally for me, which is strange)

Do you commit changes using IDE or using a cmd? Because, maybe (just a theory), the IDE does some changes before the commit and they results into the error you see

OptimumCode commented 2 months ago

Thank you very much for your help here @mikepenz. If you have time (and desire) could you please take a look at adding wasmJs target to benchmark project? As far as I can see the kotlinx-benchmark has support for wasmJs. If you don't have time or would prefer to merge these changes as is, we can do that. In this case, I will add the new target to the benchmark in a separate PR

mikepenz commented 2 months ago

Thank you very much for your help here @mikepenz. If you have time (and desire) could you please take a look at adding wasmJs target to benchmark project? As far as I can see the kotlinx-benchmark has support for wasmJs. If you don't have time or would prefer to merge these changes as is, we can do that. In this case, I will add the new target to the benchmark in a separate PR

Oh wow, thank you so much for the amazing additions and improvements to the PR. I can have a look into adding the benchmark for wasm in a new PR, and we go ahead with this one as is?

Shall we add any README changes here to note experimental wasm support or something similar?

OptimumCode commented 2 months ago

Shall we add any README changes here

Thank you for reminding me about that. Yes, we should add wasmJs to the list of supported targets in README. I don't think we should mention that it is experimental - we solved the problem with normalization and now the wasm implementation is complete.

I can have a look into adding the benchmark for wasm in a new PR, and we go ahead with this one as is?

Sure, let's do this. The only thing left in this PR is to update the README then

OptimumCode commented 2 months ago

Looks good to me. Thank you @mikepenz. I will merge this PR if you have no objections to that

mikepenz commented 2 months ago

No objections from my perspective