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

Fix gulpfile tasks in release zip, but limit packaged scripts #10311

Merged
merged 6 commits into from
Apr 21, 2022

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Apr 21, 2022

Fixes #3911

We removed the gulpfile in #10281, but after seeing #6025, I think that was the wrong move.

The reason we removed it was that it was no longer possible to run any gulp tasks from within the release zip. However, this was due to #8659 is fixed here by including .gulp.json.

Additionally, it's only possible and helpful a subset of scripts in the release zip. So this PR limits those to the following list:

  • start
  • start-public
  • clean
  • cloc
  • coverage
  • eslint
  • build-specs
  • test-*
  • prettier-check

This list is arrived at by omitting build and watch tasks, as well as the deploy-* tasks. Including generateDocumentation and build-ts is arguable since they can be run to verify the validity of the docs. But we already included the generated output, and the size of the documentation build tools are not as negligable as eslint or prettier.

In order to run those, we include include .eslintrc.json,.eslintignore, related eslint config files in the release zip (~7KB uncompressed); .prettierignore (<1KB uncompressed); and Specs/** which was previously, but I believe erroneously, excluded in #10281.

Finally, after #10293 adds back the browser spec runner and associated files, we can re-add the link to the spec runner removed from index.release.html in #10184.

@ggetz ggetz requested a review from ebogo1 April 21, 2022 15:21
@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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

Copy link
Contributor

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor comment! I agree with the scripts we left for the release zip and I tested all of them just to be safe.

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

ggetz commented Apr 21, 2022

Thanks @ebogo1! Updated.

@ebogo1
Copy link
Contributor

ebogo1 commented Apr 21, 2022

Hm, this is a pretty niche use case but I noticed that if we run npm run clean followed by npm run build-specs, the latter fails because clean removes the SpecList.js file. This happens in the non-release version as well, but if there's no way to rebuild SpecList.js from the release zip (it's part of the "build" task) maybe it's worth reconsidering how we include the build-specs task.

I think it's minor enough to not do anything about it in this PR but thought I'd mention it.

@ggetz
Copy link
Contributor Author

ggetz commented Apr 21, 2022

Good point. Maybe we should omit the clean script entirely then, since its possible to clean files which can't be re-created from within the release zip.

@ebogo1
Copy link
Contributor

ebogo1 commented Apr 21, 2022

Should the cloc script be removed too then?

@ggetz
Copy link
Contributor Author

ggetz commented Apr 21, 2022

Yeah, I think that follows. I also don't see an equivalent command in packages from comparable projects.

@ggetz
Copy link
Contributor Author

ggetz commented Apr 21, 2022

Updated once again.

@ebogo1
Copy link
Contributor

ebogo1 commented Apr 21, 2022

Cool, thanks @ggetz!

@ebogo1 ebogo1 merged commit d748986 into main Apr 21, 2022
@ebogo1 ebogo1 deleted the packaged-scripts branch April 21, 2022 21:01
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.

What tasks should be supported from within the release zip?
3 participants