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

"Just in time" builds #10538

Merged
merged 20 commits into from
Jul 21, 2022
Merged

"Just in time" builds #10538

merged 20 commits into from
Jul 21, 2022

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jul 12, 2022

This is a followup from #10399

Refactors the build scripts a bit to allow for "just in time" compilation, a method we use in other projects for internal app development.

This allows a developer to run

npm install
npm start

without having to manually build, and everything should "just work".

This is done by revising server.cjs such that requests to Cesium.js or similar build artifacts will case rebuilding incrementally if a file has changed before serving the file contents.

  • Factors out common build code from gulpfile.cjs into build.cjs
  • Updates Build Guide to explain new preferred workflow
  • Since this does not modify any of the build configuration itself from the last PR, just the workflow, there are no changes no the end user
  • HelloWorld now uses unminified Cesium so it works during development builds without having to manually build.
  • Removes file-level eslint ignores in server.js

@cesium-concierge
Copy link

Thanks for the pull request @ggetz!

  • ✔️ 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.
  • ❔ Changes to third party files were made.
    • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.

Reviewers, don't forget to make sure that:

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

@ggetz ggetz changed the base branch from main to build July 13, 2022 18:02
@ggetz ggetz marked this pull request as ready for review July 13, 2022 18:10
@ggetz ggetz requested a review from sanjeetsuhag July 13, 2022 18:10
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

@ggetz This is going to be a great quality of life update! I have a few initial comments.

gulpfile.cjs Outdated Show resolved Hide resolved
@@ -48,15 +48,16 @@
"aws-sdk": "^2.932.0",
"bitmap-sdf": "^1.0.3",
"bluebird": "^3.7.2",
"chokidar": "^3.5.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I still got the following warning when running npm install:

npm WARN deprecated chokidar@2.1.8: Chokidar 2 does not receive security updates since 2019. Upgrade to chokidar 3 with 15x fewer dependencies

Looking at node_modules/chokidar/package.json, the version seems to indicate 3.5.3, so I don't know what's causing this. Do you see this same output when running the install step?

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 not due to our immediate chokidar dependency. It's a fairly common package that others depend on as well.

You can run npm list --depth=[depth] to check the dependency tree.

In this case it looks like gulp@4.0.2 is the source of this warning. That's still the latest version unfortunately. We'll need to weigh if we want to stay will gulp, which is still a popular tool despite the lack of updates. Irregardless, that would be out of the scope of this PR.

server.cjs Show resolved Hide resolved
build.cjs Show resolved Hide resolved
gulpfile.cjs Show resolved Hide resolved
ggetz and others added 3 commits July 14, 2022 09:17
Co-authored-by: Sam Suhag <sanjeetsuhag@users.noreply.github.com>
@ggetz
Copy link
Contributor Author

ggetz commented Jul 14, 2022

That @sanjeetsuhag! I've responded to all of your feedback.

Base automatically changed from build to main July 19, 2022 18:58
@ggetz
Copy link
Contributor Author

ggetz commented Jul 19, 2022

@sanjeetsuhag Can you take another look at this when you get the chance now that #10399 is merged?

build.cjs Outdated Show resolved Hide resolved
build.cjs Outdated Show resolved Hide resolved
@ggetz
Copy link
Contributor Author

ggetz commented Jul 21, 2022

Thanks @sanjeetsuhag! I addressed your feedback.

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks @ggetz!

@sanjeetsuhag sanjeetsuhag merged commit 96b6e93 into main Jul 21, 2022
@sanjeetsuhag sanjeetsuhag deleted the build-scripts branch July 21, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

3 participants