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 require for Node.js (Fixes #8559) #8560

Merged
merged 6 commits into from
Feb 26, 2020
Merged

Conversation

jimmyangel
Copy link
Contributor

No description provided.

@cesium-concierge
Copy link

Thank you so much for the pull request @jimmyangel! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ 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.
  • Works (or fails gracefully) in IE11.

@@ -1902,7 +1902,7 @@ import TrustedServers from './TrustedServers.js';

// Specifically use the Node version of require to avoid conflicts with the global
// require defined in the built version of Cesium.
var nodeRequire = global.require; // eslint-disable-line
var nodeRequire = global.require ? global.require : require; // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we use ES6, you can just remove the nodeRequire variable and comment above it and just replace it with require directly in this function.

This will most likely have to all change again for Node 13, but there are bigger issues that need to be addressed there too.

@jimmyangel
Copy link
Contributor Author

jimmyangel commented Jan 29, 2020 via email

@jimmyangel
Copy link
Contributor Author

Hi @mramato, I made the change but there is a conflict in the contributors file :)

Thanks,

Ricardo

@mramato
Copy link
Contributor

mramato commented Feb 26, 2020

@jimmyangel I could have sworn I merged this already, but thanks again for the PR. It will be part of the CesiumJS release on Monday.

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.

3 participants