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

Bindgen: Generate spec schema if missing #7636

Closed
wants to merge 9 commits into from

Conversation

kraenhansen
Copy link
Member

@kraenhansen kraenhansen commented Apr 26, 2024

What, How & Why?

This fixes invocation of the Bindgen CLI (npx realm-bindgen) from a clean install of the repository, by adding a check for the existence of the schema before loading it from disk and executing the typescript-json-schema CLI to generate the file if it isn't there.

Because the invocation of the typescript-json-schema was now duplicated in three locations (this new place, in the CMake config for bindgen and package.json of the Realm JS repo), I refactored this into a separate generateSchema function which is exposed through a new generate-schema action on the bindgen CLI.

I considered:

  • adding this as a "postinstall" script instead, but having this just before the spec schema is loaded made more sense to me. This also makes it possible to iterate fast, by simply deleting the output file and running the CLI again.
  • calling the programatic API of typescript-json-schema instead of starting a child-process invoking its CLI, but this don't output to a file (and this is needed to get editor support for the spec) by default and it seemed a bit more involved TBH.

Ideally, this won't ever run when bindgen is invoked via CMake from an SDK, as the CMake configuration already handles this:

set(SCHEMA_FILE ${CMAKE_CURRENT_SOURCE_DIR}/generated/spec.schema.json)
add_custom_command(
OUTPUT ${SCHEMA_FILE}
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
COMMAND ${NPX} typescript-json-schema ${CMAKE_CURRENT_SOURCE_DIR}/tsconfig.json RelaxedSpec --include ${CMAKE_CURRENT_SOURCE_DIR}/src/spec/relaxed-model.ts --out ${SCHEMA_FILE} --required --noExtraProps --aliasRefs
VERBATIM
MAIN_DEPENDENCY src/spec/relaxed-model.ts
DEPENDS
# Note: The json-schema generation task is extremely slow so we don't want to rerun it any time any TS file is modified.
# Instead we assume that it will only transitively import the listed files.
# (Right now there are no imports)
)
add_custom_target(BindgenSpecJsonSchema DEPENDS ${SCHEMA_FILE})

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@kraenhansen kraenhansen self-assigned this Apr 26, 2024
@cla-bot cla-bot bot added the cla: yes label Apr 26, 2024
@kraenhansen kraenhansen added the no-jira-ticket Skip checking the PR title for Jira reference label Apr 26, 2024
@kraenhansen kraenhansen force-pushed the kh/generate-spec-schema-if-missing branch from 302b061 to c961a81 Compare April 29, 2024 08:11
Copy link

coveralls-official bot commented Apr 29, 2024

Pull Request Test Coverage Report for Build kraen.hansen_77

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 33 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.02%) to 91.111%

Files with Coverage Reduction New Missed Lines %
src/realm/array_mixed.cpp 1 94.02%
src/realm/array_timestamp.cpp 1 97.77%
src/realm/index_string.cpp 1 84.63%
src/realm/list.cpp 1 87.37%
src/realm/mixed.cpp 1 86.46%
test/test_util_network.cpp 1 95.56%
src/realm/object-store/shared_realm.cpp 2 91.9%
src/realm/sync/noinst/client_impl_base.cpp 2 82.57%
src/realm/sync/noinst/server/server.cpp 3 73.91%
src/realm/table.cpp 4 90.55%
Totals Coverage Status
Change from base Build 2580: 0.02%
Covered Lines: 217406
Relevant Lines: 238617

💛 - Coveralls

@leemaguire leemaguire removed their request for review August 16, 2024 09:01
@kraenhansen kraenhansen marked this pull request as draft August 26, 2024 12:59
@kraenhansen
Copy link
Member Author

This could probably merge, but I don't see a great need for it and it does come with some risk now that it's been a while since I wrote it.

@kraenhansen
Copy link
Member Author

Closing as it's unlikely this will ever merge.

@kraenhansen kraenhansen closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes no-jira-ticket Skip checking the PR title for Jira reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants