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.

Sign up for GitHub

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

Jump to bottom

Support wasm target #178

Merged
merged 10 commits into from
Aug 5, 2024
Merged

Support wasm target #178

merged 10 commits into from
Aug 5, 2024

Conversation

mikepenz
Copy link
Contributor

@mikepenz mikepenz commented Aug 5, 2024 •
edited by OptimumCode
Loading

  • update build setup to also include the wasm target

Note

  • The wasm target currently does not handle normalisation for IDN-hostname format

@mikepenz
Copy link
Contributor Author

mikepenz commented Aug 5, 2024 •
edited
Loading

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

@mikepenz
Copy link
Contributor Author

mikepenz commented Aug 5, 2024

Ok verified that it works 👍

@mikepenz
- update build setup to also include the wasm target
5679862 
  - FIX OptimumCode#177
  - adjust module hierarchy to not split wasm from other source sets as some dependency currently does not offer wasm support
@mikepenz
- update binary compatibility definition
a555cbb
@mikepenz mikepenz force-pushed the feature/support_wasm_targets_177 branch from 480942f to a555cbb Compare August 5, 2024 12:14
@mikepenz
Copy link
Contributor Author

mikepenz commented Aug 5, 2024 •
edited
Loading

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
Copy link
Contributor Author

mikepenz commented Aug 5, 2024

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
OptimumCode requested changes Aug 5, 2024
View reviewed changes
Copy link
Owner

@OptimumCode OptimumCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR @mikepenz. Please, take a look at the comments. If you have any questions or concerns - just let me know

...ator/src/commonMain/kotlin/io/github/optimumcode/json/schema/internal/hostname/Normalizer.kt Outdated Show resolved Hide resolved
json-schema-validator/build.gradle.kts Show resolved Hide resolved
json-schema-validator/build.gradle.kts Show resolved Hide resolved
...c/wasmJsMain/kotlin/io/github/optimumcode/json/schema/internal/hostname/Normalizer.wasmJs.kt Outdated Show resolved Hide resolved
json-schema-validator/build.gradle.kts Show resolved Hide resolved
json-schema-validator/build.gradle.kts Show resolved Hide resolved
@OptimumCode
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

mikepenz commented Aug 5, 2024

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
- mark isNormalized as internal API
e7961d6 
- add experimental annotation for wasm targets
- add also browser() to wasm target
- update *.api file
@mikepenz
Copy link
Contributor Author

mikepenz commented Aug 5, 2024 •
edited by OptimumCode
Loading

Outstanding items I'll look into:

  • wasm tests
  • wasm isNormalized implementation via JS
  • wasm benchmark
    • add wasm target to benchmark project
    • exclude wasm benchmark from execution for platforms other than Linux (like it is done for jvm target) - update benchmark workflow

@OptimumCode
Copy link
Owner

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 OptimumCode added the enhancement New feature or request label Aug 5, 2024
@OptimumCode
Copy link
Owner

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
Copy link
Contributor Author

mikepenz commented Aug 5, 2024

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
Copy link
Owner

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
Copy link
Owner

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

@mikepenz
Copy link
Contributor Author

mikepenz commented Aug 5, 2024

No objections from my perspective

@OptimumCode OptimumCode merged commit e1770b3 into OptimumCode:main Aug 5, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OptimumCode OptimumCode OptimumCode approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancement: Add new wasm target
2 participants
@mikepenz @OptimumCode