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.

Sign up for GitHub

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

Jump to bottom

Remove Autolinker from global scope #8034

Closed
wants to merge 3 commits into from
Closed

Remove Autolinker from global scope #8034

wants to merge 3 commits into from

Conversation

ajlende
Copy link

@ajlende ajlende commented Aug 1, 2019

I'm trying to build Cesium with Webpack and I ran into the same error as #7981.

TypeError: Cannot set property 'Autolinker' of undefined

I'm guessing it has something to do with the bundled code being run in strict mode, but I didn't dive very deep into why Webpack was treating this as void 0 in the bundle.

Either way, this also happens to resolve #3497, and cleans up the global scope.

Autolinker fixed this in v0.24.1, but the bundled version here is 0.17.1, so to keep it safe, I just updated the UMD header.
@cesium-concierge
Copy link

Thank you so much for the pull request @ajlende! 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.
  • ❌ Missing CLA.
  • ❔ 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.
  • ❔ 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.
@ajlende
Copy link
Author

ajlende commented Aug 1, 2019

I signed the Individual CLA and added myself to CONTRIBUTORS.md
@OmarShehata
Copy link
Contributor

Thanks @ajlende ! I can confirm we received your CLA. Looks like a pretty straightforward change, hopefully we can get it in in the next release.
@cesium-concierge
Copy link

Thanks again for your contribution @ajlende!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.
@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

Thanks for the PR @ajlende! Sorry it took so long to get you a review on this.

Instead of modifying the version we have in the repo, can you please update the library to the latest version which you said has a fix for this? Thanks!
@mramato
Copy link
Contributor

mramato commented Sep 21, 2019

@ajlende thanks again for this PR. Were in the process of migrating Cesium to ES6 modules and Autolinker has been updated to the latest version and fixed as part of that process. It should go into master and be part of the Nov 1 release.
@mramato mramato closed this Sep 21, 2019
@CesiumGS CesiumGS deleted a comment from cesium-concierge Sep 21, 2019
@bampakoa bampakoa mentioned this pull request Oct 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants
@ajlende @cesium-concierge @OmarShehata @hpinkos @mramato