Scene View better handling for not loading 3D file (including CORS)

[iremgokce]

Summary of changes 🔍

• Changed the request method from fetch to axios since fetch was returning TypeError: Failed to fetch in its catch method without detailed response information. Also since the promise returned from fetch() won't reject on HTTP error status even if the response is an HTTP 404 or 500, it would be better to use axios.
• However, even with axios's catch block (that falls out of the range of 2xx and when there is no response), it is hard to differentiate the CORS error and other network errors since there is no response returned. I also tried to use XMLHttpRequest to get the status code but it is always 0 for both CORS and network error like offline mode). Therefore, I just changed the content of the error.

References:

• https://stackoverflow.com/questions/19325314/how-to-detect-cross-origin-cors-error-vs-other-types-of-errors-for-xmlhttpreq
• https://stackoverflow.com/questions/872206/what-does-it-mean-when-an-http-request-returns-status-code-0
• https://stackoverflow.com/questions/47412384/how-to-get-the-status-code-of-a-cors-http-failed-preflight-requets
• https://stackoverflow.com/questions/5005960/xmlhttprequest-status-0-responsetext-is-empty
• https://stackoverflow.com/questions/52458341/unable-to-catch-403-in-try-catch-during-fetch
• https://axios-http.com/docs/handling_errors
• Can't catch CORS ERROR  axios/axios#838
• Set a code/status for "Network Error" axios/axios#383

Testing 🧪

• Go to ADT 3D Scene Page component and update the 3D file asset url of your scene with some file url, the container of which CORS is not enabled for the origin that you run your code. Easy way is entering a file url which is stored in a container which is different than the one you access your 3d config file and make sure CORS is not enabled for it.

Checklist ✔️

• Linked associated issue (if present)
• Added relevant labels
• Requested developer reviews
• Request UI review from design / PM
• Verify all code checks are passing

[darsney]

Looks fine, just 1 comment

[darsney]

Is there a reason we dont want to use an illustration message for this as well? Probably a different illustartion than the CORS error, but might as well use that component since the styling is nice (vs just literal text)

[iremgokce]

yeah, we can :) do you have in mind or I can ask Azima?

[ptallettms]

So far as I can tell, throwing an error looks a risky change as it promotes the fetch for the modified date to the front line, and fails the load if there's a network error fetching the modified date. I have seen MANY instances where this call fails, but the model load succeeds. I originally had something similar, but backed off that as it failed too many loads.

Please ignore this comment if that is not true.

[ptallettms]

Also this line is technically wrong, but quite nice in practice.

mod = mod ? '?' + mod : '';

If we don't get modified date, we load without a query string which will never cache bust, so if you update the model, we will never load it if the previous version is in cache.

Now actually the getModifiedDate often fails and its a pain to have to keep reloading the model as most of the time, we haven't changed the model, so the behaviour is wrong, but IMHO desirable. If a customer meets this scenario, they can upload the new model with a new name and change their 3DScenesConfig (but better would be to fix their environment so the getModifiedDate works).

If we want to cache bust if the getModifiedDate fails, then this line could be

mod = '?' + mod || new Date().toIsoString();

but I don't like it as most of the time cache busting is not what the user wants, and it fills the cache with loads of copies of the same model (but we could enable the manifest check so it never gets cached at all). I've been meaning to discuss this on standup as I heard a rumour that one of our customers was hitting this.

[iremgokce]

Hey @ptallettms, as you mentioned and we talked with Matt in dev chat, we have decided to move the CORS check logic to Babylon's loader method, but it seems they are swallowing the 403 CORS error in BABYLON.SceneLoader.Load and it is not either falling in its onError or wrapper try/catch blocks catch part. So I just contacted to Babylon team about the issue, hopefully they are going to help with that. Will keep you updated.

[iremgokce]

Looks like the line above engine.disableManifestCheck = true; is causing the BABYLON.SceneLoader.Load method not surfacing the network error in its onError callback. I asked it to Babylon people and waiting for their reply if they are interconnected. @ptallettms why do we need disabling manifest check, was it for caching related?

[iremgokce]

Hey @ptallettms what is you final input on this mod related line? Is mod = '?' + mod || new Date().toIsoString(); better option now? I will keep it as it was originally and let me know if we want to change it, I can do after this PR, since the scope of this PR is handling CORS related error, not improving caching method.

[ptallettms]

Whilst technically incorrect, I'd leave it as it is as that is what the user probably intended.

[iremgokce]

thanks for your reply!

[iremgokce]

Okay there has been a fix from Babylon team to expose the network error in the loader even caching is enabled @ptallettms @darsney: BabylonJS/Babylon.js#12647 whenever they merge, I will bump up the Babylon version in our package.json and move the CORS/network checking logic to the loader instead of getModifiedTime.

[iremgokce]

Okay there has been a fix from Babylon team to expose the network error in the loader even caching is enabled @ptallettms @darsney: BabylonJS/Babylon.js#12647 whenever they merge, I will bump up the Babylon version in our package.json and move the CORS/network checking logic to the loader instead of getModifiedTime.

UPDATE: Babylon team is going to publish the fix with Thursday's release: 5.11.0, then I am going to bump Babylon version and move the network error handling logic to loader instead: https://github.com/BabylonJS/Babylon.js/releases/tag/5.11.0

[github-actions]

🎉 This PR is included in version 1.0.0-beta.262 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀