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

Update npm dependencies and regenerate 3DTiles/glTF classes. #705

Merged
merged 3 commits into from
Aug 24, 2023

Conversation

kring
Copy link
Member

@kring kring commented Aug 16, 2023

I just updated npm dependencies by removing package-lock.json from the root and from tools/generate-classes and running npm install again. And then I regenerated both the 3D Tiles classes and the glTF classes to make sure the generator was still working. Regenerating picked up some doc changes, and there are also some very minor formatting changes, possibly due to a clang-format upgrade.

There are no functional changes here, but the doc improvements are useful and this should also resolve the dependabot PRs recently opened. My motivation in doing this at all was mostly just to make sure I can still run the generator successfully before I start making changes to it, but the result in this PR is worth merging.

Copy link
Contributor

@csciguy8 csciguy8 left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for this. The changes look good, but did have some related questions,

  • Is there documentation how to generate these classes? If not, can we get a blurb about it?
  • Is generation a step we can do on CI so we never have to manually run it?

@kring
Copy link
Member Author

kring commented Aug 24, 2023

Is there documentation how to generate these classes? If not, can we get a blurb about it?

I added some doc about this to the main README.md.

Is generation a step we can do on CI so we never have to manually run it?

Well... maybe! As I mentioned offline, there are some downsides to this:

  1. Developers would need Node.js installed in order to build cesium-native. Currently it's not required.
  2. We'd need to lock down the particular version of the specs we're generating code from, so that changes to the specs can't break our build.
  3. It takes a little time to run the generator.

Even with these caveats, it could be a worthwhile tradeoff. Maybe. Definitely not as part of this PR, though.

@kring
Copy link
Member Author

kring commented Aug 24, 2023

Ready for another look.

@csciguy8
Copy link
Contributor

Looks good. I created a new issue to explore moving to CI.

@csciguy8 csciguy8 merged commit 9239add into main Aug 24, 2023
2 checks passed
@csciguy8 csciguy8 deleted the regenerate-code branch August 24, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants