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

Adds workspaces #10824

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Adds workspaces #10824

merged 1 commit into from
Nov 1, 2022

Conversation

sanjeetsuhag
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag commented Sep 27, 2022

CesiumJS is an extensive library with a lot of functionality that, depending on the app or use case, may go unused. Given that it is a 3D geospatial engine, some users may use the whole stack by embedding the CesiumViewer in their application utilizing all the features that the Timeline and Animation widgets bring, some users may only want a lean interface through the CesiumWidget and some users may only want to use the geospatial math library in Core, for instance. Given the diversity of our user community and the domains CesiumJS is used for, the benefits of having a modularized structure of packages become increasingly apparent: users can choose to depend on only what they want to use in their application. The benefits go beyond producing a leaner bundle; it eliminates the need to install all of the direct dependencies that are synced with the existing cesium npm package,. It also reduces the surface area of the API which the user is supposed to learn along with providing flexibility in selecting when/how they want to update their dependencies.

smaller_packaegs

We plan on maintaining our regular monthly release schedule for CesiumJS. Users may continue to use the combined CesiumJS release, however, anyone that would benefit from ingesting smaller, more specific packages would be able to use them when they’re released alongside the combined cesium package. To start, these smaller packages take the form of “engine” and “widgets”. We use npm workspaces to impose a monorepo structure. These packages can be found in the root level packages directory and will be published under the @cesium scope, i.e. @cesium/engine and @cesium/widgets. As compared to the combined CesiumJS package, which includes pre-bundled artifacts such as Cesium.js, index.js and index.cjs, these newer packages will be shipped in a lean configuration, as ES modules with TypeScript definitions, as is the standard for modern packages. This will allow users to employ the bundling tools of their choice for their applications and take advantage of features like tree-shaking, or dead code elimination. Also, as per npm best practice, these packages will follow semantic versioning, which let users clearly understand the scope of changes between versions and prevent cases where allowing npm to automatically update versions would result in broken builds.

The engine package is the core runtime components of CesiumJS, including everything except the Widgets folder found in the combined CesiumJS release. The one exception here is that CesiumWidget has been moved from the widgets to engine. This provides a lean and minimal way of adding CesiumJS to the browser. So, if you’re only using the engine package, you can get started with the following snippet:

import { CesiumWidget } from @cesium/engine”;
import @cesium/engine/Source/Widget/CesiumWidget.css”;

const widget = new CesiumWidget(“cesium-container”);

The widgets package contains all the different UI widgets that are part of the combined CesiumJS package. These include the Viewer. Timeline, Animation etc. along with the different mixins. To get started with the widgets package, you can use the following snippet:

import { Viewer } from @cesium/widgets”;
import @cesium/widgets/Source/widgets.css”;

const viewer = new Viewer(“cesuim-container”);

Since widgets depends on engine, the engine package will automatically be installed when you install widgets.

In terms of what these changes look like in the repository or the combined release, all source files from the root level Source folder have been moved into the respective package folder. For example, Scene.js has moved from Source/Scene/Scene.js to packages/engine/Source/Scene/Scene.js. And Viewer.js has moved from Source/Widgets/Viewer/Viewer.js to packages/widgets/Source/Viewer/Viewer.js. Any direct dependencies included in the source code have been moved to the ThirdParty folder within each package’s Source folder.

When working within the repository, each package also comes with it’s own simple scripts:
build - Creates the package’s entry module index.js, bundles the Specs
build-ts - Generates the TypeScript definitions file
test - Runs tests
coverage - Generates coverage report

To minimize impact on applications that depend on the combined CesiumJS release structure, static assets found in Assets, ThirdParty and Widgets are still copied to the root level Source folder at the time of publishing.

These changes are another important step of our plan to continue modernizing the CesiumJS library and it builds on the previous improvements to our source code, tooling and build processes. In the future, we hope to be able to break the packages down along even finer lines, such as publishing a core package that contains all the geospatial math classes like Cartesian3 etc.


Folder Structure Changes

  • The relevant source files have been moved to packages/engine/Source or package/widgets/Source.
  • The relevant spec files have been moved to packages/engine/Specs or package/widgets/Specs.
  • CesiumWidget has been moved to packages/engine/Source/Widget, as opposed to packages/widgets/Source.
  • Shared spec files, such as createScene remain in the root level Specs folder, along with Data.

Testing

  • The original testing workflow for CesiumJS remains unchanged. npm run test or testing using Jasmine from the browser still work the same way.
  • To enable testing of each workspace independently, we bundle the Specs for each workspace. When bundling the workspace specs, we simply bundle the karma-main.js with the SpecList.js to generate an ESM bundle for testing with Karma. npm run test --workspaces or npm run test -w @cesium/widgets allow for testing workspace specs.
  • Static assets used while testing (Assets, ThirdParty etc) are server from Build/CesiumUnminified.

TypeScript

  • The root level Source/Cesium.d.ts remains unchanged.
  • Each workspace generates its own index.d.ts file using its own tsd-conf.json file.
  • All public modules exported from @cesium/engine are imported into @cesium/widgets.
  • Since the KmlTour.prototype.play function takes in a Viewer parameter, which is now in @cesium/widgets, a manual type definition is added to the index.d.ts:
  /**
   * @property scene - The scene in the widget.
   */
  export type Viewer = {
      scene: Scene;
  };

CI

  • CI now uses Travis Build Stages to run the following tasks in parallel:
    1. Linting, Coverage, Deployment
    2. Release Tests
  • This only cuts down on ~1.5 minutes of CI time, however, this is only a starting point. In a future PR, we may parallelize this a little more. That may require revamping of our deployment process.

Build Artifacts

  • The build artifacts for Cesium and CesiumUnminified remain unchanged.
  • To ensure that users who depend on the Source folder to import static assets, the Assets, ThirdParty, Widgets (only the CSS files) and Workers are copied into the Source folder.
  • When publishing the workspace packages, we only intend on publishing the following files:
    • @cesium/engine
      • index.js
      • index.d.ts
      • README.md
      • LICENSE.md
      • Source
      • Build/Workers
      • Build/ThirdParty
    • @cesium/widgets
      • index.js
      • index.d.ts
      • README.md
      • LICENSE.md
      • Source

Release Process

  • The monthly release cycle for CesiumJS will continue as before.
  • Each workspace package abides by semantic versioning, starting with version 0.1.0 as as prerelease version.
  • The CHANGES.md should be grouped by workspaces.

Status

  • generate index.js for each package
  • generate types.d.ts for each package
  • generate documentation
  • generate build for each package
  • get test working for packages
  • get test working for CesiumJS
  • get live rebuilds working for all packages
  • get Apps /Sandcastle working
  • cleanup gulpfile.js/build.js/server.js
  • add copyright banner
  • update public API
  • generate separate coverage paths
  • revise package.json and dev documentation
  • update CI
  • test ingestion of packages (Webpack, NodeJS, create-react-app)

Package Consumption Tests

create-react-app
  • From the root of the cesium repository, run the following command:
npm link --workspaces
  • In a separate folder, run the following command:
npx create-react-app cesium-react-app --template typescript
cd cesium-react-app
# Bundling Cesium with webpack requires additional configuration
npm run eject
npm link @cesium/engine @cesium/widgets
npm i -D copy-webpack-plugin
  • In config/webpack.config.json file and add the following plugins:
const cesiumEngineSource = 'node_modules/@cesium/engine/Source';
const cesiumWidgetsSource = 'node_modules/@cesium/widgets/Source';
const cesiumWorkers = '../Build/Workers';
/// In the resolve object:
fallback: { "https": false, "zlib": false, "http": false, "url": false },
/// In the plugins object:
new webpack.DefinePlugin({
	CESIUM_BASE_URL: JSON.stringify('')
}),
new CopyWebpackPlugin({
	patterns: [
          { from: path.join(cesiumSource, cesiumWorkers), to: 'Workers' },
          { from: path.join(cesiumSource, 'Assets'), to: 'Assets' },
          { from: path.join(cesiumSource, 'ThirdParty'), to: 'ThirdParty' },
          { from: path.join(cesiumSource, 'Widget'), to: 'Widgets/CesiumWidget' },
          { from: cesiumWidgetsSource, to: 'Widgets' }
	]
}),

You should be able to use the CesiumWidget or CesiumViewer in your React app.

@cesium-concierge
Copy link

Thanks for the pull request @sanjeetsuhag!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

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.

Exciting, thanks @sanjeetsuhag! Here's my thoughts on an initial pass.

  • Do we still produce Source/Cesium.js? Since this is the recommended way to load CesiumJS in app that use build tooling, it would definitely be a breaking change if this went away in the next release without warning. At the very minimum, we should be able to provide a warning in CHANGES.md or a deprecation period.

  • Likewise, required static files such as Assets, Widget CSS, and ThirdParty are often copied from Source folder (for example, as recommended in cesium-webpack-example). To avoid a breaking change, we'll need to have a plan for these paths.

  • Related, package.json will need to be edited to ensure that it is not pointing to any non-existent Source paths.

  • One thing this brings up is directory casing consistency. All the new directories are lowercase/camelcase, but that conflicts with our existing precedent of uppercase/Pascal case. This leads to some "weird" paths like packages/engine/Source. I imagine at least all new paths should chose one style and stick with it. Any thoughts?

  • We'll need LICENSE.md and README.md files for both packages (I didn't see at least the license file in the TODO list). I'm happy to help get content together for the READMEs.

  • Only the engine workspace creates its own build artifacts at the moment. Unsure if this is even needed or if we should just let a user's bundling tools do the job.

    I think we should aim for the minimum number of build artifacts possible, at least to start. ESM is what most newer packages support.

  • For things like generating TypeScript definitions, unless we adopt a copy-paste strategy, this has the potential to increase CI time.

    If we notice a major slowdown, it might be apt to only run these on CI for the main branch, but let's leave that as a last resort.

Apps/CesiumViewer/CesiumViewer.css Outdated Show resolved Hide resolved
Specs/BadGeometry.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
packages/widgets/package.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@sanjeetsuhag
Copy link
Contributor Author

Do we still produce Source/Cesium.js? Since this is the recommended way to load CesiumJS in app that use build tooling, it would definitely be a breaking change if this went away in the next release without warning. At the very minimum, we should be able to provide a warning in CHANGES.md or a deprecation period.

Yes, npm run build will generate Source/Cesium.js.

Likewise, required static files such as Assets, Widget CSS, and ThirdParty are often copied from Source folder (for example, as recommended in cesium-webpack-example). To avoid a breaking change, we'll need to have a plan for these paths.

Upon running npm run build, static assets are copied from the workspaces to the Source folder. The .gitignore has been updated to ignore these artifacts.

One thing this brings up is directory casing consistency. All the new directories are lowercase/camelcase, but that conflicts with our existing precedent of uppercase/Pascal case. This leads to some "weird" paths like packages/engine/Source. I imagine at least all new paths should chose one style and stick with it. Any thoughts?

I think the packages/<workspace> folders can be the exceptions here. I think it's preferable to keep the packages names and paths consistent.

I think we should aim for the minimum number of build artifacts possible, at least to start. ESM is what most newer packages support.

To run the npm run test, we do need to generate a bundle, so that is the bare minimum needed. When publishing the package, I agree that we should only publish ESM.

We'll need LICENSE.md and README.md files for both packages (I didn't see at least the license file in the TODO list). I'm happy to help get content together for the READMEs.

Yeah, for the README.md as well as the package.json's description property, I may need your help.


@ggetz One public API issue is the KmlFlyTo class that directly references Viewer. From within engine, we can add a similar method to work with CesiumWidget instead, but question is where does the viewer related code go?

@ggetz
Copy link
Contributor

ggetz commented Oct 7, 2022

I think the packages/ folders can be the exceptions here. I think it's preferable to keep the packages names and paths consistent.

I agree for existing paths (like Source/Cesium.js), but I disagree for new paths. I think it's the best time to update path as no one should be relying on them yet.

One public API issue is the KmlFlyTo class that directly references Viewer. From within engine, we can add a similar method to work with CesiumWidget instead, but question is where does the viewer related code go?

Is this the code in KmlTour?

@sanjeetsuhag
Copy link
Contributor Author

I agree for existing paths (like Source/Cesium.js), but I disagree for new paths. I think it's the best time to update path as no one should be relying on them yet.

Oh, alright, I misunderstood. Sure, I think we can do that for the packages.

Is this the code in KmlTour?

Yep.

@ggetz
Copy link
Contributor

ggetz commented Oct 7, 2022

The root of the issue here is that the different levels of abstraction (DataSources and Widgets in the case) are not respected, so some refactoring may be the solution. (Source code changes themselves can probably go into before workspaces, and may be easier to review in a separate PR).

For KmlTour, it appears that while the parameter is often passed around, the only time viewer is actually used is with viewer.scene.camera in playEntry. CesiumWidget has access the scene property as well, so we could probably use that instead. Is there any other viewer use that needs to be accounted for here?

@ggetz
Copy link
Contributor

ggetz commented Oct 7, 2022

Yeah, for the README.md as well as the package.json's description property, I may need your help.

Do you want me to drop these in this branch, or maybe open a PR into this one?

@sanjeetsuhag
Copy link
Contributor Author

This is strange - in 5aa0fff, one of the tests failed in CI, but it's showing up as CI passed.

@ggetz
Copy link
Contributor

ggetz commented Oct 10, 2022

one of the tests failed in CI, but it's showing up as CI passed.

@sanjeetsuhag Is there a failTaskOnError missing somewhere?

@sanjeetsuhag
Copy link
Contributor Author

Based on the CI log, the failTaskOnError option was set: https://app.travis-ci.com/github/CesiumGS/cesium/jobs/585078672#L2972

@sanjeetsuhag sanjeetsuhag marked this pull request as ready for review October 10, 2022 13:03
@ggetz
Copy link
Contributor

ggetz commented Oct 10, 2022

@sanjeetsuhag I pushed up README.md files for each packages, as well as description values in each package.json file.

A few pieces of feedback and questions:

  • Each package should have an .npmignore file which should specify any files or artifacts that should not be published-- For example /Build/Specs and /Specs.
  • I think LICENSE.md can be paired down for the individual packages. For instance, they shouldn't need to include the Apps dependencies, and widgets shouldn't need to include the engine dependencies.
  • Both new package.json files should specify the module and sideEffects fields, and widgets will need to include the exports field so that CSS files can be imported.
  • Shouldn't the d.ts files both be index.d.ts for consistant with index.js?
  • testWidgets and testEngine seem to have a lot of redundancy in their implementation. If we were to introduce more packages, adding additional functions may not scale very well. Is it possible to have a single testPackage function that would take command line arguments? You may be able to pull the --workspaces value directly.
  • I saw errors while running npm run test -w @cesium/engine. And only one set of renderer tests ran (this was after building with npm run build -w @cesium/engine)
  • Do you have list of expected steps we'll want to add the release guide?
  • Could we possibly take advantage of build stages to run steps in parallel?

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 12, 2022

@sanjeetsuhag I pushed up README.md files for each packages, as well as description values in each package.json file.

Thank you!

Each package should have an .npmignore file which should specify any files or artifacts that should not be published-- For example /Build/Specs and /Specs.
I think LICENSE.md can be paired down for the individual packages. For instance, they shouldn't need to include the Apps dependencies, and widgets shouldn't need to include the engine dependencies.
Both new package.json files should specify the module and sideEffects fields, and widgets will need to include the exports field so that CSS files can be imported.

Agree on all points, I'll do another pass over all the package.json files.

Shouldn't the d.ts files both be index.d.ts for consistant with index.js?

Yep, as recommended by the TS docs, updated.

testWidgets and testEngine seem to have a lot of redundancy in their implementation. If we were to introduce more packages, adding additional functions may not scale very well. Is it possible to have a single testPackage function that would take command line arguments? You may be able to pull the --workspaces value directly.

Yeah, I meant to refactor this too. It's been all consolidated under one test function now. Also, all scripts in the workspaces package.jsons now run gulp tasks with the following syntax: gulp <task> --workspace <workspace>.

I saw errors while running npm run test -w @cesium/engine. And only one set of renderer tests ran (this was after building with npm run build -w @cesium/engine)

I did a lot of cleanup with test and coverage, I think these should all be working now.

Could we possibly take advantage of build stages to run steps in parallel?

This is very interesting. Although I am not sure how best to structure this for efficiency. For comlpeteness, I think I'd like to run all the scripts for each workspace, however, that's kinda already done (to build Cesium.js or Cesium.d.ts, you have to run those tasks for each workspace first. test and coverage are kinda covered too. I wonder if we can run the coverage and test --release task in parallel here(?).

@sanjeetsuhag
Copy link
Contributor Author

@ggetz Thoughts on the latest CI run? It used 2 build stages (Tests and Deploy). The tests run the coverage, test --release and test --workspaces in parallel. And the deploy task is the normal deploy stuff. I created a build-release task to generate release builds without having to run build-ts and build-docs. One issue is the deploy-s3 - since we need to run the coverage to generate the report, we might have to create a gulp task to upload that to S3 first. I tried just calling deploy-s3 twice and that did not work.

@ggetz
Copy link
Contributor

ggetz commented Oct 12, 2022

One issue is the deploy-s3 - since we need to run the coverage to generate the report, we might have to create a gulp task to upload that to S3 first. I tried just calling deploy-s3 twice and that did not work.

deploy-s3 is dependent on coverage and make-zip. I think these would need to be grouped in the same job. There should be no need to run deploy twice.

I'm not sure if every part of build-release is needed for the release tests. We don't need to build Unminified Cesium for them to run, right? If it helps organize things, the Node.js smoke-screen tests do not have to necessarily be grouped together, but they do need to be grouped with a corresponding build task, depending on whether they are testing dev or production.

Lastly, how much benefit do we get from running the workspace tests separately from coverage? Coverage should run all tests as if they are not release tests, right? It think it would still make sense to verify the workspaces with a smokescreen test similar to the existing ESM smokescreen test.

Based on the above, I think it make sense to have one job which runs prepare, the linting steps, make-zip, coverage, and deploy. In parallel, there could be another job which runs the release tests, and one which runs the workspace tests if we deem them valuable. For the parallelization to be beneficial enough to maintain, we should see a speedup in total time though.

And lastly, if these changes make sense outside of the scope of adding workspaces, they should go in their own PR.

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.

A few thought on the CSS...

@@ -1,7 +1,7 @@
@import url(./lighterShared.css);
@import url(./Animation/lighter.css);
@import url(./BaseLayerPicker/lighter.css);
@import url(./CesiumWidget/lighter.css);
@import url(../../engine/Source/Widget/lighter.css);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work when using the @cesium/widgets package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As tested it seems to work.

packages/engine/Source/Widget/CesiumWidget.js Show resolved Hide resolved
@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 17, 2022

@ggetz For documenting changes to the public API from now on, I think we should start grouping changes by workspaces in CHANGES.md, see Babylon's CHANGELOG.md for example. Also, just to clarify, workspace packages will only be published on npm and not separately on GitHub too, correct?

@ggetz
Copy link
Contributor

ggetz commented Oct 17, 2022

Also, just to clarify, workspace packages will only be published on npm and not separately on GitHub too, correct?

Correct. The github releases would be the same as before-- The code snapshot plus the generated release zip.

@ggetz
Copy link
Contributor

ggetz commented Oct 18, 2022

I think we should start grouping changes by workspaces in CHANGES.md.

This should start in the next release, right? If so, let's make that change here.

Additionally, this change should be listed under Major Announcements section in CHANGES.md. It should include announcing the new packages and how it will affect users. The later should be minimal, but it may be worth calling out that the Source files have changes locations in case anyone was depending on that behavior.


The steps for bumping the package numbers in each workspace is a bit manual as it stands now. Could we automate or simplify that process at all, perhaps using the npm version command?

We could patch, minor, or major based on semver depending on the changes in that release. Since we tag manually later in the release guide process, let's be sure to include the --no-git-tag-version since, if run in a git repo, it will also create a version commit and tag.

We could then specify a postversion script in each workspace which could update the root package.json to match. For instance, I got something working with the following gulpfile task:

export const postversion = function () {
  const workspace = argv.workspace;
  if (!workspace) {
    return;
  }

  // After the workspace version is bumped by `npm version`, 
  // update the root `package.json` file to depend on the new version
  const directory = workspace.replaceAll(`@${scope}/`, ``);
  const workspacePackageJson = require(`./packages/${directory}/package.json`);
  const version = workspacePackageJson.version;

  packageJson.dependencies[workspace] = version;
  return writeFile("package.json", JSON.stringify(packageJson, undefined, 2));
}

What do you think?


Use npm publish -w <WORKSPACE> in the repository root (not the unzipped file directory) to publish the workspace. Repeat this step for each updated workspace.

Instead of repeating for each workspace, can we use the --workspaces flag to publish all the packages at once? From the npm docs:

It's also possible to use the workspaces (plural) configuration option to enable the same behavior but running that command in the context of all configured workspaces. e.g:

npm run test --workspaces


Source/Wokers is empty except for a package.json file after running build. Is that intentional?


Did you happen to add any smokescreen tests for the packages, similar to Secs/test.mjs? I think that would help verify everything is working in case of any changes.

@sanjeetsuhag
Copy link
Contributor Author

This should start in the next release, right? If so, let's make that change here.

Ok, I'll update CHANGES.md.

Could we automate or simplify that process at all, perhaps using the npm version command?

I like the workflow suggested here. I'll update the Release Guide accordingly.

Instead of repeating for each workspace, can we use the --workspaces flag to publish all the packages at once

This was a deliberate choice to minimize the impact of issues encountered during the release process. I still think it's better to do this step by step, since we only have two workspaces to worry about right now, but I will think more about this.

Source/Wokers is empty except for a package.json file after running build. Is that intentional?

Good catch. I think Source/Workers should not be there, since users are expected to import those from the Build folder. What is missing the Source/ThirdParty folder. That I believe users can import from Source, so I need to copy that back in.

Did you happen to add any smokescreen tests for the packages, similar to Specs/test.mjs? I think that would help verify everything is working in case of any changes.

I'll add those.

CHANGES.md Outdated Show resolved Hide resolved
@ggetz
Copy link
Contributor

ggetz commented Oct 19, 2022

Instead of repeating for each workspace, can we use the --workspaces flag to publish all the packages at once

This was a deliberate choice to minimize the impact of issues encountered during the release process. I still think it's better to do this step by step, since we only have two workspaces to worry about right now, but I will think more about this.

That's fair. Should each command be listed in release guide then, to reduce any user error?

@ggetz
Copy link
Contributor

ggetz commented Oct 19, 2022

When I run npm pack at the root directory to produce a copy of what will be published, I see packages/engine/Specs and packages/widgets/Specs are published. The original Specs folder is not included, so I don't believe these should be either.

@mramato
Copy link
Contributor

mramato commented Oct 21, 2022

I only looked at this at a very high level and while it will take a while to get used to the new source layout, I was happy to see the mechanics of building/running themselves are largely unchanged. I'm very much looking forward to this new era of Cesium and even more modularity in the future. 🎉

That being said, the PR description seems very incomplete. I know it links out to the original discussion, but we can't expect everyone to go read the entire thread and figure out where things ended up. I would recommend editing the primary PR description to include a comprehensive summary of everything that was done and why. This basically becomes the basis for the blog post on the subject so it's very useful in the long run. Not that I have all the answers but see the two below PRs for how I've documented giant changes in the past:

It's possible this change is not as big as it feels so the summary does not need to be overly long, I just feel like there are a lot of details that were left out. I've never used npm workspaces for example, so perhaps there are assumptions being made about the knowledge of the average Cesium dev?

A few other questions from my first 5 minutes:

  • Why is CesiumWidget in engine? That feels like something that would lead to a lot of confusion in the future. It throws the entire widgets naming into question.
  • The npm packages look wrong. Running npm pack -w @cesium/engine after running make-zip produced a tgz file that is missing the built Cesium and also includes things like the Specs folder. Have we manually evaluated the contents of these zips for correctness?
  • Likewise, have we done any sort of comparison with the generated doc and TS files to ensure we haven't introduced any hard to miss bugs?
  • Are there any plans to clearly identify which module something lives in in the documentation? There doesn't appear to be a way to show the doc for "just engine". Is there?
  • Maybe this change happened as part of another PR, but I noticed npm start is always building stuff now. Is that just doing an in-memory just-in-time esbuild build? or is it writing stuff out to disk as well? I just wonder what happens after I generate the release zip and then if I run npm start does that modify things? (which would affect npm pack?)

I'm sure I'll have a million more questions, and I haven't looked at the code. This is just my impression from my first 10 minutes.

Nice job overall.

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 21, 2022

Thanks for taking a look @mramato!

I would recommend editing the primary PR description to include a comprehensive summary of everything that was done and why. This basically becomes the basis for the blog post on the subject so it's very useful in the long run.

This is a fair point, and I'll update the PR description to include more context and be more comprehensive about the nature of the change.

Why is CesiumWidget in engine? That feels like something that would lead to a lot of confusion in the future. It throws the entire widgets naming into question.

This was done to allow users to have a minimal way of getting Cesium to the browser, without having the overheard of all the other elements and dependencies that CesiumViewer and family bring. I agree that the naming is a big confusing - in an ideal world, CesiumWidget would be called Viewer and vice-versa. That would jive well with the package names too, but since everyone uses Viewer, I think it may be too big a change.

The npm packages look wrong. Running npm pack -w @cesium/engine after running make-zip produced a tgz file that is missing the built Cesium and also includes things like the Specs folder.

The inclusion of the built Specs was a mistake, but the omission of an IIFE bundle isn't. We're planning on shipping these smaller packages as minimally as possible, allowing users to bundle stuff as they need. The Build folder in @cesium/engine still contains the ThirdParty, Widget and Workers to help users set the CESIUM_BUILD_URL to that and have things just work.

have we done any sort of comparison with the generated doc and TS files to ensure we haven't introduced any hard to miss bugs?

This Cesium.d.ts.diff reveals that the only changes here, aside from the ones to CesiumWidget, are the changes to the paths for directly importing just one file.

Are there any plans to clearly identify which module something lives in in the documentation? There doesn't appear to be a way to show the doc for "just engine". Is there?

Currently, no. Users can see the package name in the file name, but that's about it:

Screen Shot 2022-10-21 at 9 55 56 AM

Maybe this change happened as part of another PR, but I noticed npm start is always building stuff now. Is that just doing an in-memory just-in-time esbuild build? or is it writing stuff out to disk as well?

Yes, esbuild enables use to quickly rebuild when a change happens. The output now is just a little bit noisy, and I think I am leaning towards turning that off, since it doesn't add any benefit to the user. But the generated bundles are in-memory and not written to file.

I just wonder what happens after I generate the release zip and then if I run npm start does that modify things? (which would affect npm pack?)

This actually bit us during the release process, hence the fix introduced in #10843

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 21, 2022

@ggetz I think we're ready for a prerelease now. Here's a very minimal wepack app that demonstrates the usage, as shown in the package READMEs.

  1. From the workspaces branch, run the following commands:
git clean -x -d -f
npm install
npm run make-zip
npm pack --workspaces
  1. Copy the .tgz files generated into the folder that you unzipped above.
  2. Run the following commands:
npm install
npx webpack --config webpack.config.js
npx http-server ./dist --cors
  1. Go to http://localhost:8080

Note: In src/index.js, a @cesium/engine only and a @cesium/engine + @cesium/widgets app examples are shown. Uncomment on or the other one to test. As zipped, the @cesium/engine + @cesium/widgets is shown. I've also added terrain to demonstrate the Workers are working.

@mramato
Copy link
Contributor

mramato commented Oct 25, 2022

  • Since the KmlTour.prototype.play function takes in a Viewer parameter, which is now in @cesium/widgets, a manual type definition is added to the index.d.ts:

This feels like something that should have it's own clean-up issue because the workaround is fine, but points to bad abstraction.

@mramato
Copy link
Contributor

mramato commented Oct 25, 2022

Thanks for the update PR description @sanjeetsuhag, looks great!

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 26, 2022

@ggetz The latest commits seem small, but are important, so I want to highlight the changes:

  • 87b405f - For the @cesium/engine package, previously the ThirdParty and Widget folders were also included in the Build folder. These are not needed, only the Workers are. The Assets and ThirdParty can be copied directly from Source instead.
  • 257a699 - Removes the packages to .npmignore so that we're not publishing extra files. @cesium/engine and @cesium/widgets are direct dependencies of the cesium package, so we don't need to publish sources for those as they will be installed by anyone installing cesium. I ran npm pack and tested it against the current main branch of cesium-webpack-example and everything worked as expected.

gulpfile.js Outdated Show resolved Hide resolved
@thw0rted
Copy link
Contributor

I have a couple questions, apologies if they're already covered upthread -- I tried to skim through everything but there's a lot...

  • Is the old package still going to be published in tandem with the scoped ones?
    • If so how will semver work? (How will I know which @cesium/engine matches a given version of cesium?)
    • If so, will both import {Entity} from "cesium" and import {Entity} from "cesium/Source/DataSources/Entity" still work going forward?
  • Will the new packages support both of the above import syntaxes?

.prettierignore Outdated Show resolved Hide resolved
@sanjeetsuhag
Copy link
Contributor Author

Thanks for taking a look at this PR, @thw0rted! I was hoping to get your feedback to catch any issues, especially with the TypeScript definitions.

Is the old package still going to be published in tandem with the scoped ones?

Yes, the cesium package will still be published on a monthly basis.

If so how will semver work? (How will I know which @cesium/engine matches a given version of cesium?)

cesium will not follow SemVer, only the dependencies will. So, there is a possibility that different versions of cesium may depend on the same version of @cesium/widgets (if there are no changes made to the source code in that particular package), for example.

If so, will both import {Entity} from "cesium" and import {Entity} from "cesium/Source/DataSources/Entity" still work going forward?

Only static assets such as images from Assets, WASM binaries from ThirdParty and CSS files from Widgets will be shipped in the Source folder. So, the import {Entity} from "cesium/Source/DataSources/Entity" will not work.

Will the new packages support both of the above import syntaxes?

Yes. You can do either one of the following:

import { Cartesian2 } from "@cesium/engine";

or

import Cartesian2 from "@cesium/engine/Source/Core/Cartesian2.js";

@ggetz
Copy link
Contributor

ggetz commented Oct 27, 2022

If so, will both import {Entity} from "cesium" and import {Entity} from "cesium/Source/DataSources/Entity" still work going forward?

Right now, even in main, some tooling will throw an error when importing paths that are not explicitly exported by package.json. So import {Entity} from "cesium/Source/DataSources/Entity" may not work in some cases back in main. Assuming that did work with your tooling, the new path would be import {Entity} from "cesium/packages/engine/Source/DataSources/Entity".

@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Oct 27, 2022

the new path would be import {Entity} from "cesium/packages/engine/Source/DataSources/Entity".

@ggetz Would this still work as of the changes in 257a699 that removed the packages folder from the cesium package when it is published?

I'd also like to learn more about the motivations behind why a user might want to import a specific file instead of just the standard package import.

@ggetz
Copy link
Contributor

ggetz commented Oct 27, 2022

Would this still work as of the changes in 257a699 that removed the packages folder from the cesium package when it is published?

Ah true, my mistake.

I'd also like to learn more about the motivations behind why a user might want to import a specific file instead of just the standard package import.

I'd defer that question to @thw0rted, but I expect it may have to do with tree shaking. But that should be resolved as of the use of sideEffects in package.json.

gulpfile.js Outdated Show resolved Hide resolved
@ggetz
Copy link
Contributor

ggetz commented Oct 27, 2022

Running npm start seems to wipe some of the contents of Build/CesiumUnminified, such that running tasks after the server is shut down, such as build-apps, fail. Does a fix similar to https://github.com/CesiumGS/cesium/pull/10843/files need to be incorporated?

Also built Sandcastle appears to be missing some CSS:
image

@thw0rted
Copy link
Contributor

Re: importing from a deep path, I wasn't especially concerned, it's just that my IDE sometimes suggests them, they work for some people, and they'll break after this change. Might at least be worth mentioning in the patch notes.

@sanjeetsuhag
Copy link
Contributor Author

Running npm start seems to wipe some of the contents of Build/CesiumUnminified, such that running tasks after the server is shut down, such as build-apps, fail. Does a fix similar to https://github.com/CesiumGS/cesium/pull/10843/files need to be incorporated?

Updated. The CesiumJS build from server.js is now output to Build/CesiumDev.

Also built Sandcastle appears to be missing some CSS:

Fixed.

@ggetz
Copy link
Contributor

ggetz commented Oct 28, 2022

@sanjeetsuhag Can we add a link to this PR in CHANGES.md? Your PR description is a great resource before the blog post is officially published.

@mramato
Copy link
Contributor

mramato commented Oct 28, 2022

Just a suggestion and I'm not sure when we are planning to merge it, but it might make a lot of sense to squash these changes into a single commit when we are ready to merge, that way it's easy to checkout before/after the re-org in case we run into weird bugs and want to check if this is what caused it. (We could probably also just use a tag too, but I always like the cleanliness of a single PR, especially when it comes to git bisect)

@sanjeetsuhag
Copy link
Contributor Author

055e753: This commit removes the individual module declarations in Cesium.d.ts. that allowed importing definitions for a single module.

@ggetz
Copy link
Contributor

ggetz commented Nov 1, 2022

@sanjeetsuhag Can you please merge in main and squash your commits?

@sanjeetsuhag
Copy link
Contributor Author

@ggetz Pulled from main and squashed the commits.

@ggetz
Copy link
Contributor

ggetz commented Nov 1, 2022

Awesome, thanks @sanjeetsuhag! I'll merge and then tag this change as post-workspaces for easier merges with existing branches. The tag before this will be 1.99. So to ease merging in this change, other branches should merge 1.99, then merge workspaces and update any new paths.

@ggetz ggetz merged commit 27abb99 into main Nov 1, 2022
@ggetz ggetz deleted the workspaces branch November 1, 2022 20:09
@ggetz ggetz mentioned this pull request May 16, 2023
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.

5 participants