-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Create @cesium/utils package #12581
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
base: main
Are you sure you want to change the base?
Create @cesium/utils package #12581
Conversation
Thank you for the pull request, @jfayot! ✅ We can confirm we have a CLA on file for you. |
Hi @jfayot, I'm happy to see the interest in this! We've been eyeing doing something similar for math and other non-rendering 3D and geospatial classes. A few thoughts up front:
|
Thanks @ggetz for your review.
Sure I can try to stick back to esbuild, but I guess that the new Sandcastle will also be using vite as build system, so it's not really like we'll add it specifically for @ceium/utils. And here, vite is only used for new packages, @cesium/engine and @cesium/widget still rely on esbuild. This would be the opportunity to progressively modernize cesium's build system. Let me know if you still prefer to have @cesium/utils use esbuild.
I was thinking of the following files to move to @cesium/utils:
I've excluded everything that uses Math or that has an indirect dependency on Math, as it would create circular dependency. Resource and buildModuleUrl are in this case. One potential solution would be to create another package @cesium/http-helpers that would depend on @cesium/math and @cesium/utils
Do you mean that those deprecations should be done prior to this PR? |
41fa80f
to
cc9cff0
Compare
I think we can circle to back to that discussion after we sort out the structure of the proposed package. I don't think it's a matter of esbuild or vite being more modern, but what are the granularity of options needed. 🙂
I think that would be helpful over including the deprecations in this PR. That way they can be reviewed separately and don't need to depend on the rest of the code reorg here, which will likely have more discussion. I'll follow up with a few more thoughts on what belong in each package, but I agree there should likely be a distinction between what goes in this proposed |
I have not yet looked at all changes and the details of all things that are proposed to be pulled out. One quick, drive-by comment is that
look like they could be a candidate for an own package, or, depending on whether there are considerations to have some additional structure within the packages, some Also, such a |
Description
This PR is an experimental approach to tackle different subjects through the creation of an additional package @cesium/utils (I realize that it could be renamed @cesium/utility to match cesium native component):
For the moment, only a few files have been moved to the new package @cesium/utils:
Corresponding Specs have been moved too, but unit test FeatureDetection / "detects WebGL2 support" had to be removed as it pulled unwanted dependency Scene.
There have been some imports reordering during the process, which makes quite a lot of modified files. This is because I temporarily used organize-imports-cli. This could be added as a lint-stage.
For the documentation generation, I'm still using jsdoc comment headers, but as it is with typescript files, I had to use the jsdoc-babel plugin. It works quite well, and allows to keep consistent doc across js and ts. But it could be interesting to evaluate other tools such as typedoc which are specifically made for typescript.
For the declaration types, I'm using the one bundled by dts vite plugin, and then inserting it at the end of Cesium.d.ts (see function createTypeScriptDefinitions).
If this approach is something you'd like to continue with, let me know, so that we can agree on what to put inside @cesium/utils, and help me identify what's missing in the build / ci scripts (for instance debug pragma strip out...)
Issue number and link
Addresses following issues:
Testing plan
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change