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

Revised our public CesiumJS & webpack guide #28

Merged
merged 99 commits into from
Oct 26, 2021
Merged

Conversation

srothst1
Copy link
Contributor

@srothst1 srothst1 commented Aug 17, 2021

The current CesiumJS and Webpack guide is outdated. Even if a user follows the instructions exactly, they will end up with numerous errors and broken code. This branch is a revised version of the original guide. See TUTORIAL.md for the guide. The final step where CesiumJS is implemented may require some more time.

Feedback of all kinds would be appreciated! I am still somewhat new to webpack, so suggestions regarding webpack are particularly welcomed.

@shehzan10

webpack.config.js Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
Copy link

@liuqun liuqun left a comment

Choose a reason for hiding this comment

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

Is it assumed that only the newest version of Cesium and Webpack would work together?

@srothst1
Copy link
Contributor Author

srothst1 commented Aug 18, 2021

@liuqun Thank you for the feedback. I believe my most recent commit resolves the issues that you brought up. Does the final section of TUTORIAL.md covering CesiumJS look okay to you?

We should encourage users to use the newest version of both CesiumJS and webpack.

Copy link

@liuqun liuqun left a comment

Choose a reason for hiding this comment

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

I like this TUTORIAL.md.
And, maybe we should add a URL link for the readers to view the newest TUTORIAL.md on Github?

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
TUTORIAL.md Outdated Show resolved Hide resolved
@srothst1
Copy link
Contributor Author

@liuqun

I like this TUTORIAL.md.
And, maybe we should add a URL link for the readers to view the newest TUTORIAL.md on Github?

Thank you for the feedback. I incorporated the changes that I feel are appropriate for this tutorial. Given the nature of this tutorial, the breadth and depth of the information provided must not overwhelm a new user. Thus, features like offline functionality are not necessary here.

Copy link

@liuqun liuqun left a comment

Choose a reason for hiding this comment

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

@srothst1
Copy link
Contributor Author

@liuqun sure thing!

@liuqun
Copy link

liuqun commented Aug 24, 2021

@srothst1
Copy link
Contributor Author

While what we have now is probably sufficient for getting community members up and running with CesiumJS and webpack, we should include information on how to import ES6 modules directly. This information could go in the Advanced webpack configurations and Resources section.

package.json Outdated Show resolved Hide resolved
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.

Thanks @srothst1! Great to see this tutorial getting some attention and happy that we're using the up to date versions of everything here.

I have one concern with how we are importing CesiumJS. I think we should get that cleared up before diving into any of the particulars.

src/index.html Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@srothst1
Copy link
Contributor Author

@ggetz thank you for taking the time to look over this pull request. Once completed, this will be a terrific resource for the CesiumJS community! I agree completely, let's ensure that we are importing CesiumJS correctly and then do a deep dive into the actual tutorial.

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.

Awesome, thanks @srothst1 ! I have a few more comments. Additionally you might want to check the CI logs and see what is causing it to error.

src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
srothst1 and others added 2 commits August 26, 2021 10:02
Co-authored-by: Gabby Getz <gabby@cesium.com>
@srothst1 srothst1 self-assigned this Aug 26, 2021
@srothst1
Copy link
Contributor Author

@ggetz thank you for adding some additional comments! In terms of the Travis CI build, I am currently getting the following error.

npm ERR! missing script: release

I am unsure why the build is running the line $ npm run release. Do you have any suggestions? I am going to continue looking into the issue later this afternoon.

@ggetz
Copy link
Contributor

ggetz commented Aug 26, 2021

@srothst1 Travis is configured in .travis.yml.

It looks like you changed the names of the scripts in https://github.com/CesiumGS/cesium-webpack-example/pull/28/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R6. You'll need to update that file to use the new script names.

src/index.js Outdated Show resolved Hide resolved
@srothst1
Copy link
Contributor Author

@ggetz thanks for the clarification - I just updated .travis.yml. I believe that npm run build and npm run start are the appropriate scripts to run.

@shehzan10
Copy link
Member

@srothst1 Thanks for the changes. @ggetz Feel free to merge if all the tech work is reviewed and fixed.

@ggetz
Copy link
Contributor

ggetz commented Oct 26, 2021

Tech aspect is all ready to go from my view. Thanks @srothst1 for seeing this through!

@ggetz ggetz merged commit ca9c608 into main Oct 26, 2021
@ggetz ggetz deleted the cesium-webpack-example-2021 branch October 26, 2021 20:16
@TJKoury
Copy link

TJKoury commented Nov 4, 2021

C137.js is now open-sourced and Apache-2.0 licensed.

@thw0rted
Copy link

thw0rted commented Nov 5, 2021

Thanks for the pointer TJ! I'm looking over it now. Where's the best place to ask questions / leave comments? I'd love to see some of the ideas you use incorporated into the way Cesium handles their own packaging...

@TJKoury
Copy link

TJKoury commented Nov 5, 2021

@thw0rted Start the discussion on the repo itself, open an issue.

@maylortaylor
Copy link

Uncaught Error: Cannot find module '..\..\node_modules\cesium/Build/CesiumUnminified/index.cjs' at the point below:
module.exports = webpack_require("../../node_modules/cesium sync recursive")(path.join(
__dirname,
"Build/CesiumUnminified/index.cjs"
));

@TJKoury
Copy link

TJKoury commented Aug 7, 2023

Just FYI, we deprecated c137.js since no one was using it and the changes to the build process obsoleted our approach. The core Cesium team is aware of these requirements and are working to incorporate some of the concepts I explored into the current official build process.

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.