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

Remove import.meta.url usage #8314

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Remove import.meta.url usage #8314

merged 1 commit into from
Oct 28, 2019

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Oct 24, 2019

import.meta.url is only at stage 3 of the standards track and tools like webpack and babel do not have good support. Not all browsers do either. I ran into problems just trying to update the cesium-webpack-example and Edge won't even part JS code with import.meta.url in it.

This PR removes import.meta.url usage and just does a little extra work so that all of our developer setups properly set CESIUM_BASE_URL. Most ES6 development setups would require setting CESIUM_BASE_URL anyway, so this doesn't change much of anything for our end users.

Folks using combined versions of Cesium.js are unaffected.

I also tightened up our regex for getBaseUrlFromCesiumScript because Cesium.js is always named Cesium.js if it is available, which wasn't the case years ago when this code was written. This should prevent accidental detection of the wrong script, for example CesiumViewer.js would get selected as a false positive. This type of check is only valid for non-ES6 module usage anyway, since there is no script tag inserter for individual ES6 modules.

Fixes #8251 You can now load Cesium Viewer unbuilt in Edge (but it takes forever). But the real reason for this is improving compatibility with the current tooling ecosystem.

import.meta.url is only at stage 3 of the standards and tools like webpack
and babel do not have good support. Not all browser do either.  I ran into
problems just trying to update the cesium-webpack-example because of it
and Edge won't even part JS code with import.meta.url in it.

This PR removes import.meta.url usage and just does a little extra work
so that all of our developer setups properly set CESIUM_BASE_URL. Most ES6
development setups would require setting CESIUM_BASE_URL anyway, so this
doesn't change much of anything for our end users.

Folks using combined versions of Cesium.js are unaffected.

I also tightened up our regex for `getBaseUrlFromCesiumScript` because
Cesium.js is always named Cesium.js if it is available, which wasn't the
case years ago when this code was written.  This should prevent accidental
detection of the wrong script, for example `CesiumViewer.js` would get
selected as a false positive.  This type of check is only valid for
non-ES6 module usage anyway, since there is no `script` tag inserter for
individual ES6 modules.

Fixes #8251 You can now load Cesium Viewer unbuilt in Edge (but it takes
forever)
@cesium-concierge
Copy link

Thanks for the pull request @mramato!

  • ✔️ 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.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@mramato
Copy link
Contributor Author

mramato commented Oct 24, 2019

The easiest way to test this (other than letting CI pass) is running all of the built versions of everything

@mramato
Copy link
Contributor Author

mramato commented Oct 24, 2019

This was also needed for CesiumGS/cesium-webpack-example#13 to work with the latest versions of everything.

@OmarShehata
Copy link
Contributor

So CESIUM_BASE_URL tells CesiumJS where it's being loaded from when running the unbuilt version, so that it can correctly loads resources like images/css etc that are under Source ? The only thing I'm confused about is:

Most ES6 development setups would require setting CESIUM_BASE_URL anyway

I was unaware of this. How would I know that I need to do this as a developer using CesiumJS? This only matters if you're importing the unbuilt CesiumJS into your ES6 app, which isn't as common as just using the built version?

I'm just trying to figure out how common the need to know to set CESIUM_BASE_URL as a global variable is.

@mramato
Copy link
Contributor Author

mramato commented Oct 25, 2019

So CESIUM_BASE_URL tells CesiumJS where it's being loaded from when running the unbuilt version, so that it can correctly loads resources like images/css etc that are under Source ?

No, CESIUM_BASE_URL tells CesiumJS where all of it's external files, such as WebWorkers, approximateTerrainHeights.json, skybox textures, etc.. live. It has nothing to do with built vs unbuilt.

I was unaware of this. How would I know that I need to do this as a developer using CesiumJS?

If we can't auto-detect the correct location, we throw an error and spit out instructions in the console.

I'm just trying to figure out how common the need to know to set CESIUM_BASE_URL as a global variable is.

If you are just copying the Build folder and including Cesium.js, then you don't need to set it because we can find it automatically. We look for the location of Cesium.js script in the HTML and create a relative path from that. If you are building an app from modules, then there is no relation to where the Workers, etc.. live on your server vs where your compiled JS lives, that's when it needs to be set. It just so happens that in most modern tooling setups, this is the case.

@OmarShehata
Copy link
Contributor

Thanks for the explanation Matt. I think the only thing I have is to consider changing:

Unable to determine Cesium base URL automatically, try defining a global variable called CESIUM_BASE_URL.

to:

Unable to determine Cesium base URL automatically. To fix this, define the URL where CesiumJS's assets are served in a global variable called CESIUM_BASE_URL.

The former explains what the problem is, but it doesn't give me enough info to fix it (what should be in the CESIUM_BASE_URL?) Although "CesiumJS's assets" is also kind of vague.

Most JS libraries won't tend to have external assets/workers like we do, but those that do will tend to have this information in their "webpack project template" or equivalent tutorial. So I think that may be the best way to document this information in the future.

Let me know what you think. I'd be happy to merge as-is and open an issue for documenting what BASE_URL is for and when to set it.

@mramato
Copy link
Contributor Author

mramato commented Oct 28, 2019

Let's merge and handle the doc update in a separate issue. I think we can get to a slightly longer explanation that doesn't leave any room for confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge won't parse JS code that uses import.meta.url
3 participants