Skip to content

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Sep 19, 2025

Description

This PR contains updates to the build process for Sandcastle. These were largely inspired/required by the ability to build sandcastle for the ion-sdk repo/library.

  • New buildStatic and createSandcastleConfig allow building sandcastle from outside the package directory. This should reduce the pseudo-dependency that the sandcastle package has on knowing it's nested in the CesiumJS repo.
    • This function should also now make it much more clear which files/resources are "external" by just setting all the necessary paths to access them wherever it's hosted.
    • I still need to actually relocate the configs into the larger CesiumJS build processes but I wanted to confirm the approach first. I think we'll be able to remove all the vite configs except the dev one after this.
  • The engine and widgets packages are now bundled into a single file to make it easier to load them in the browser.
    • Using this and an importmap we're also now using the version of cesium that imports/re-exports from engine/widgets to avoid duplicate cesium versions in the browser
    • This also enables easier importing of the ion-sdk packages when we need to
    • The types import was also made dynamic based on build-time paths. This can probably still be improved in the future but works and I've tested it with the ion-sdk
  • Development sandcastles will no longer be included in the production build. This was a small config change while I was already in there

Issue number and link

Part of #12894

Testing plan

  • This was mostly an internal, build system, related change so there's not much to test in the app.
  • Make sure npm run build-sandcastle works from the project root
  • Make sure things are built to the correct, expected locations

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz September 19, 2025 19:41
Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Sep 22, 2025

Thanks for the refactoring @jjspace!

I'm not sure if this is covered by the TODO item in your PR description, but the deployed app is not loading:

image

In broad strokes, the approach you describe makes sense to me. I'm taking a closer look at the code itself now.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

I think this is definitely a step in the right direction. I'm glad to see static files being handled separately from the vite build where possible.

I have a few clarifying questions, and I think we should be able to simplify the process a bit more. Let me know what you think.

// https://vite.dev/config/
const baseConfig: UserConfig = {
/** @type {UserConfig} */
const baseConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why now JS instead of TS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To enable easy imports outside of the sandcastle package without requiring it be compiled with typescript first.
/scripts/build.js -> /packages/sandcastle/buildStatic.js -> /packages/sandcastle/vite.config.js


config.define = {
...config.define,
__VITE_TYPE_IMPORT_PATHS__: JSON.stringify(typePaths),
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we'er bending over backwards to get the type definition locations. I'm thinking this would be a lot easier if we build out the definitions relative to the bundled files—Then, we wouldn't need to configure the path to Source/Cesium.d.ts and instead could get the path at runtime based on the resolved module URLs from the import maps:

const cesiumModulePath = import.meta.resolve("cesium");
const tsdPath = new URL("./Cesium.d.ts", cesiumModulePath).href;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of this change but it would involve some larger changes with the rest of our build systems. Happy to take a look at it at some point but I think I'd prefer to leave this as is for this current PR. Maybe it's rather verbose in the config but at least it should be clear what it's doing.

* @param {string[]} [filenames]
* @returns {PluginOption}
*/
export const insertImportMap = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, is there no standard plugin for this? If not, are we perhaps missing something that serves a similar function in vite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be a couple plugins that do this but none are popular or seem to be actively maintained. This code is loosely based on this one but mostly just using the Plugin API. It's a small requirement and could be made to fit our use cases so I felt it was better to just fully "own" the code and implement the plugin ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

At first brush, that all sounds fine. The potential gotcha I'm worried about is that

but none are popular or seem to be actively maintained

is because we're missing a simpler or more integrated way of getting to the same result. Nothing is top of mind, but I may investigate further.

My ask is just that we've done the due diligence on alternative approaches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My ask is just that we've done the due diligence on alternative approaches.

AFAIK I have. This seems like the cleanest and easiest solution for what we need. If we didn't want to request external files then the import map wouldn't be needed and everything could be bundled by vite. I assume that's why this is not a more "common" process.

Comment on lines 28 to 30
// IN DEV THIS DOES NOT COPY FILES it simply sets up in-memory server routes
// to the correct locations on disk in the parent directories
// This config should not be used for the actual build, only development of Sandcastle itself
Copy link
Contributor

Choose a reason for hiding this comment

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

I think my question is more along the lines of: If this is only being used for the development server, why are we using the viteStaticCopy plugin at all? Is there a more standard way to configure these paths? Should they be aliased instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is only being used for the development server, why are we using the viteStaticCopy plugin at all?

Largely because we already have it as a dependency. It didn't feel necessary to add another plugin or system when this one works well for our usecase. It's also how we tell people to set up a vite application in our own example repo

Is there a more standard way to configure these paths? Should they be aliased instead?

Not that I've been able to find. This is once again a slightly "special" case for us because we want to statically host files on the dev server that are not in the public directory. Most vite articles say to "just add your files to public and it'll work" but that kinda defeats our purpose of using the local files directly. The alternative seems to be to set up a custom plugin with middleware/routing on the serve part of vite to "host" the expected files at the expected paths. We could implement this ourselves but the vite static copy plugin already does all this heavy lifting for us. server.proxy does not work, it proxies to a different hostname which is not what we want. resolve.alias does not work either, it seems to only care about the built files which these are not.

@jjspace
Copy link
Contributor Author

jjspace commented Sep 23, 2025

I'm not sure if this is covered by the TODO item in your PR description, but the deployed app is not loading:

Mismatch in my local CI build test not including the trailing / in the path that the CI job does. Fixed now!
I'm looking through your other comments now.

@javagl
Copy link
Contributor

javagl commented Sep 23, 2025

Also see #12911

@javagl
Copy link
Contributor

javagl commented Sep 25, 2025

I just reopened #12910 : There is another import at

const config = await import(configPath);
that will have to become
const config = await import(pathToFileURL(configPath).href);

(I could create a PR, but... let's not over-formalize the process here...)

@jjspace jjspace mentioned this pull request Sep 26, 2025
6 tasks
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

@jjspace I took another pass, this time a bit closer. Let me know the status of this PR, as I think you mentioned there are still updates incoming?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does bucket.html live in templates, but not standalone.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Largely historical reasons. I believe there were some issues with routes before. Possibly resolved now but I would like to leave it alone in this pr as it's working as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this more as I was working and I'm assuming that this might have been an effort to keep the urls "cleaner"? Instead of sandcastle.cesium.com/templates/standalone.html it only has to be sandcastle.cesium.com/standalone.html. Regardless the shorter url should still work in case anyone has saved it.

I did some work to simplify paths across the board by now utilizing relative paths as much as possible. I coupled this with setting the <base> element for the standalone so it behaves like it's the templates/bucket.html file. This seems to simplify the build and still leave this path working as expected so I think it's a good compromise

Comment on lines 7 to 11
function getCesiumVersion() {
const data = readFileSync(join(__dirname, "../../package.json"), "utf-8");
const { version } = JSON.parse(data);
return version;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the value of Cesium version be passed as an option rather than autodetected here? getCesiumVersion is now defined in multiple places, despite serving the same purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option would be to set a env variable with the value of cesium version. We already do this for building the docs.

I'm leaning towards this route as I think we could streamline several areas of our build processes and CI workflows if we use an environment variable instead. We could extract and set the env var as part of the postinstall step to ensure it's only run once and before all of the other build steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the value of Cesium version be passed as an option rather than autodetected here?

Yes, I thought the same thing, already updated.

Another option would be to set a env variable with the value of cesium version

I think my new approach/opinion is to leave any env variable reliance out of the sandcastle scripts. This could be used in the main build/gulpfile scripts but I think that can be separate from these changes if we want to go that route

config.define = {
...config.define,
__VITE_TYPE_IMPORT_PATHS__: JSON.stringify(typePaths),
__COMMIT_SHA__: JSON.stringify(commitSha ?? undefined),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to use process.env.GITHUB_SHA directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may not always want to use it. For example in Prod we don't want to show it but it's currently set in the github workflow. Doing it this way makes it customizable and explicit from wherever we call this function. Using the env variable directly creates more, almost, "side effect" logic that people have to be aware of


const copyPlugin = viteStaticCopy({
targets: [
{ src: "templates/Sandcastle.(d.ts|js)", dest: "templates" },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the TS build output to the public directory instead to save a step?

Comment on lines +10 to +23
export const cesiumPathReplace = (cesiumBaseUrl) => {
return {
name: "custom-cesium-path-plugin",
config(config) {
config.define = {
...config.define,
__CESIUM_BASE_URL__: JSON.stringify(cesiumBaseUrl),
};
},
transformIndexHtml(html) {
return html.replaceAll("__CESIUM_BASE_URL__", `${cesiumBaseUrl}`);
},
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

What all do we need __CESIUM_BASE_URL__ after using import maps? Consider also that we can dynamically get the paths relative to the Cesium ESM import at runtime based on the import map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the cesium base url for all of the assets and files that CesiumJS loads. This is setting up the app the same way we recommend/require everyone else to do. I think we should leave it like this

*/

/**
* @typedef {Object<string, ImportObject>} ImportList
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @typedef {Object<string, ImportObject>} ImportList
* @typedef {Record<string, ImportObject>} ImportList

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is JS doc not TS, Record doesn't exist in JS. Intellisense would probably pick it up fine in most environments and I'm already stretching the rule a little with the @import lines but it was intentional to use Object not Record here.

Comment on lines 29 to 33
{ src: join(__dirname, `${cesiumSource}/ThirdParty`), dest: cesiumBaseUrl },
{ src: join(__dirname, `${cesiumSource}/Workers`), dest: cesiumBaseUrl },
{ src: join(__dirname, `${cesiumSource}/Assets`), dest: cesiumBaseUrl },
{ src: join(__dirname, `${cesiumSource}/Widgets`), dest: cesiumBaseUrl },
{ src: join(__dirname, `${cesiumSource}/*.(js|cjs)`), dest: cesiumBaseUrl },
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we copy the Cesium build artifacts to S3 as part of the GitHub Actions workflow? That would be more consistent with our other deployments.

Copy link
Contributor Author

@jjspace jjspace Sep 29, 2025

Choose a reason for hiding this comment

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

It might be possible to do that but then it would leave the built files not working by themselves. This way it creates a single directory that can be copied to S3, just like the other build processes do. Plus this way we can test it locally by doing PROD=true npm run build-sandcastle and then hosting those static files with something like npx http-server

(FYI This file and configuration has been removed in favor of incorporating it into the gulpfile like our other build steps)

@jjspace
Copy link
Contributor Author

jjspace commented Sep 29, 2025

Let me know the status of this PR, as I think you mentioned there are still updates incoming?

@ggetz yes, sorry for the delay it took a bit longer getting things together than I thought it would. I've just pushed an update to extract the build configs out to the top level and eliminate all of the "pseudo-dependency" on being in the cesium repo. This includes now exporting the buildStatic and buildGallery functions for use in the cesium build scripts/gulpfile.
I'm looking through your new comments now, I think some might already be addressed by this change

@jjspace
Copy link
Contributor Author

jjspace commented Sep 30, 2025

I need to finalize how this works in the release zip file. Currently it can't "install" the sandcastle package because it's not included. We need to build it before the zip and/or sort out which files to include.

@jjspace jjspace mentioned this pull request Sep 30, 2025
6 tasks
# Download zip from the Github release and unzip to Build/release/
# Publish that unzipped code to the bucket for https://cesium.com/downloads/cesiumjs/releases/[version]/... urls
# Publish the documentation files to the bucket for https://cesium.com/learn/cesiumjs/ref-doc/... urls
# Publish the simple viewer app
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggetz do you know where the CesiumViewer app is deployed/hosted?

@jjspace jjspace mentioned this pull request Oct 7, 2025
6 tasks
@jjspace
Copy link
Contributor Author

jjspace commented Oct 7, 2025

@ggetz I've pushed a handful of updates with a focus on stability and simplification to support our various build methods.

One of the bigger/more important changes is that Sandcastle is now relying on relative urls for it's assets and resources as much as possible for non-PROD builds. This means the same build should work locally and in CI and deployed to the releases CDN page. PROD builds still set absolute paths because they're intended to fully "bundle" all necessary assets.

Alongside that change I also had to modify the standalone.html file a little further to account for the relative routes which will be different compared to bucket.html.

Testing different builds:
Run git clean -dxf between each to start as if you just cloned the repo

  • Zip file
    • npm install && npm run make-zip
    • Extract to some temp folder
    • run npm install && npm start in said folder
  • "cold start" or Fresh install flow
    • npm install
    • npm start - this should now also build sandcastle if it wasn't already
  • PROD deployment
    • npm install
    • PROD=true npm run website-release
    • PROD=true npm run build-ts
    • PROD=true npm run build-apps
    • Inside Build/Sandcastle2 run npx http-server to check that it all works as an isolated set of static files

I also want to share a small server script that's useful for checking the ci route and release route both work locally

ci-server.js

Just put this file at the root of the repo named ci-server.js or something and run it in place of npm start
Then navigate to

import path from "path";
import yargs from "yargs";
import express from "express";

function excludeRoutes(excludedRoutes) {
  return function middleware(req, res, next) {
    if (excludedRoutes.some((route) => req.path.startsWith(route))) {
      return res.status(404).send();
    }
    next();
  };
}

const argv = yargs(process.argv)
  .options({
    port: {
      default: 8080,
      description: "Port to listen on.",
    },
  })
  .help().argv;

(function () {
  const app = express();

  app.use(
    "/downloads/cesiumjs/releases/1.133",
    express.static(path.resolve(".")),
  );
  app.use(
    "/cesium/branch",
    // the CI deploy excludes these folders, don't serve them here
    excludeRoutes([
      "/.git/",
      "/.github/",
      "/.husky/",
      "/.vscode/",
      "/Build/Coverage/",
      "/Build/CesiumDev/",
      "/Build/Specs/e2e",
      "/Documentation/",
      "/node_modules/",
      "/scripts/",
      "/Tools/",
    ]),
    express.static(path.resolve(".")),
  );

  const server = app.listen(argv.port, "localhost", () => {
    console.log(
      "Cesium mock ci server running locally.  Connect to http://localhost:%d/cesium/branch/",
      server.address().port,
    );
  });

  server.on("close", function () {
    console.log("Cesium deployment test server stopped.");
    process.exit(0);
  });

  let isFirstSig = true;
  process.on("SIGINT", function () {
    if (isFirstSig) {
      console.log("\nCesium deployment test server shutting down.");

      server.close();

      isFirstSig = false;
    } else {
      throw new Error("Cesium deployment test server force kill.");
    }
  });
})();

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.

3 participants