search

CesiumGS / cesium-webpack-example

The minimal recommended setup for an application using Cesium with Webpack.
Apache License 2.0
248 stars 160 forks source link

Revised our public CesiumJS & webpack guide #28

Closed srothst1 closed 3 years ago

srothst1 commented 3 years ago

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

srothst1 commented 3 years ago

@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.

srothst1 commented 3 years ago

@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.

srothst1 commented 3 years ago

@liuqun sure thing!

liuqun commented 3 years ago
srothst1 commented 3 years ago

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.

srothst1 commented 3 years ago

@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.

srothst1 commented 3 years ago

@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 commented 3 years ago

@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.

srothst1 commented 3 years ago

@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.

srothst1 commented 3 years ago

My immediate goal is to make sure that the code is working properly. Once this is complete, I will continue editing the guide. I believe that I have incorporated all of the feedback related to index.js and index.html.

I am currently struggling to resolve a Travis CI-related issue. The command npm run release is giving me the following error.

[webpack-cli] Invalid configuration object. Webpack has been initialized using a configuration object that does not match the API schema.

I added the following lines to package.json

    "release": "node_modules/.bin/webpack --config webpack.release.config.js",
    "serve-release": "node_modules/.bin/webpack-dev-server --config webpack.release.config.js --content-base /dist"

and created the file webpack.release.config.js. I have also tried strategies suggested in the following threads but have had no luck.

I am relatively new to Travis CI, which makes this issue particularly challenging for me.

ggetz commented 3 years ago

@srothst1 Do you get the same errors when running npm run release on your machine? Or is this only happening in Travis?

srothst1 commented 3 years ago

@ggetz I get the same errors when I run npm run release on my machine.

ggetz commented 3 years ago

OK, so that means the problem is not with Travis. If you get that script running on your machine, it should work similarly in Travis.

Do you plan on having a release configuration as part of this tutorial? A release configuration omits any debugging tools and may have some additional optimization as compared to the dev config. If you do not plan on including it, you can omit the release script from both package.json and travis.yml entirely.

srothst1 commented 3 years ago

@ggetz thank you for the additional information. For now, including a release configuration does not seem necessary. Are there any additional edits that should be made to the code part of this tutorial? If not, I will resume editing TUTORIAL.md.

ebogo1 commented 3 years ago

On a nitpicky side note, I'd force push the final changes to TUTORIAL.md to squash the 26+ commits with the same or nearly identical messages..

srothst1 commented 3 years ago

@argallegos thank you very much for the detailed feedback on TUTORIAL.md. I implemented most of the changes that you suggested :rocket:

@ebogo1 @ggetz can you please confirm that everything looks correct before I merge?

shehzan10 commented 3 years ago

can you please confirm that everything looks correct before I merge?

@srothst1 Please do not self-merge this. Please make sure that one of the PR reviewers are merging once all the requirements are met.

srothst1 commented 3 years ago

@ggetz I tested out the project after adding all of the recent changes and it still works :+1:

ggetz commented 3 years ago

Thanks @srothst1 ! This is looking good to me. Thanks for your patience with the review too. Since this is a tutorial, its important we have all of our ducks in a row.

Is there anyone else that needs an eye on this or is it ready to be merged?

shehzan10 commented 3 years ago

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

ggetz commented 3 years ago

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

TJKoury commented 3 years ago

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

thw0rted commented 3 years ago

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 commented 3 years ago

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

maylortaylor commented 1 year ago

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 commented 1 year ago

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.