-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Sandcastle Reborn #12574
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
Sandcastle Reborn #12574
Conversation
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
Wahoo! @jjspace could you get this PR targeting to a feature branch for sandcastle v2? That way we can have smaller, iterative PRs as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great so far! I haven't reviewed any of the app code itself, since as I understand it is very much in flux. But I do have some thoughts on the overall configuration.
@ggetz I was expecting this branch/PR to be that feature branch. I was thinking of developing directly here until the build system is mostly sorted then do smaller PRs into this branch for review as needed. Where you thinking differently? |
To keep things clean, let's target a new staging branch. That way we can merge this and keep the PR list tidy. Well... tidier. |
@ggetz CI is now working properly on this branch https://ci-builds.cesium.com/cesium/sandcastle-reborn/Apps/Sandcastle2/index.html There may be some more things to clean up but I believe that everything with the builds is set up to work in whatever environment we put it in, CI being the hardest. Might be able to make it "nicer" but what's there should work so I think it's good enough for now. Please take a peek at the new vite configs specifically and make sure CI, local (through |
For testing I found it really helpful to mimic the way it's set up on the CI server to avoid having to push it up and wait for the actual CI deploy every change. In whatever starting directory you want: mkdir local-ci-test && cd local-ci-test
mkdir cesium && cd cesium
mkdir branch && cd branch
ln -s [path to cesium]/Apps
ln -s [path to cesium]/Build
ln -s [path to cesium]/Source
cd ../../
npx http-server --port 8081 Then navigate to http://localhost:8081/cesium/branch/Apps/Sandcastle2/ (a |
Thanks or the updates @jjspace! I'll take a review pass on this shortly. I assume this PR is good to be marked "Ready for review"? |
@ggetz yes I think for now this is good to review, I can make further changes in subsequent PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jjspace Nice, this all looks like a solid base to build on top of. I have a few comments, but have indicated where I wouldn't expect some them to hold up this PR getting merged.
@@ -34,6 +35,9 @@ packages/widgets/Build/** | |||
packages/widgets/index.js | |||
packages/widgets/Source/ThirdParty/** | |||
|
|||
packages/sandcastle/node_modules/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a blanket **/node_modules/**
rule work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically yes but I think we specifically want it to be obvious when dependencies are messed up and it creates a node_modules
inside the other two packages since that's undesired.
@@ -34,6 +35,9 @@ packages/widgets/Build/** | |||
packages/widgets/index.js | |||
packages/widgets/Source/ThirdParty/** | |||
|
|||
packages/sandcastle/node_modules/** | |||
Apps/Sandcastle2/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we signal that this is built output by putting it in a Build
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly but potentially not very easily given how files are accessed, specifically the sample data. I'd like to leave it here for now to mimic the existing sandcastle as much as possible.
rules: { | ||
...reactHooks.configs.recommended.rules, | ||
"react-refresh/only-export-components": [ | ||
"warn", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "warn" or "error"? Right now, warnings will not be indicating when running our eslint
command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. This was a direct copy out of the generated file from the vite setup just to make it work with the flat eslint config. I honestly didn't really look at what was set or not set yet, completely open to review if you want to take a stab at changing this and extracting like you mention in the other comment.
...[...tseslint.configs.recommended].map((config) => ({ | ||
// This is needed to restrict to a specific path unless using the tseslint.config function | ||
// https://typescript-eslint.io/packages/typescript-eslint#config | ||
...config, | ||
files: ["packages/sandcastle/**/*.{ts,tsx}"], | ||
})), | ||
{ | ||
// This config came from the vite project generation | ||
files: ["packages/sandcastle/**/*.{ts,tsx}"], | ||
languageOptions: { | ||
ecmaVersion: 2020, | ||
globals: globals.browser, | ||
}, | ||
plugins: { | ||
"react-hooks": reactHooks, | ||
"react-refresh": reactRefresh, | ||
}, | ||
rules: { | ||
...reactHooks.configs.recommended.rules, | ||
"react-refresh/only-export-components": [ | ||
"warn", | ||
{ allowConstantExport: true }, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder... To help isolate things a bit more, can we defined the following config in a standalone file and just import it here? I may look into this a bit. No need to hold up the PR.
export default defineConfig(() => { | ||
const cesiumBaseUrl = `${process.env.BASE_URL}Build/CesiumUnminified`; | ||
|
||
console.log("Building Sandcastle with base url:", cesiumBaseUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this console.log
left here intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was helpful to be able to see in the CI output if/when there are issues. Maybe not needed but also don't think it hurts?
@ggetz ready again |
Awesome, thanks @jjspace! |
Description
Starting work on rebuilding Sandcastle. Currently this branch has the scrapped together hackathon code. This branch and PR will act as a staging branch for the work involved in the whole process. See #12566 for more details
packages/sandcastle
to encapsulate the new Vite + React + Typescript projectApps/Sandcastle2
(temporary path)vite
dev server and the static files from the normal CesiumJS dev server should behave the same. This requires a bit of extra config to make sure routes for assets and sample data line up. Should be working now but may require some finagling later if we change routes.TODO:
Issue number and link
Fixes #12566
Testing plan
From the top level
npm run build-sandcastle
npm start
http://localhost:8080/Apps/Sandcastle2/index.html
From inside
packages/sandcastle
for direct development of sandcastlenpm run dev
for the dev server buildhttp://localhost:5173/
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change