Swap node internal packages out for fetch in Resource
#11773
Conversation
| return new Uint8Array(loadWithHttpResponse).buffer; | ||
| } | ||
| } | ||
|
|
||
| function loadWithHttpRequest( |
There was a problem hiding this comment.
The data and overrideMimeType arguments are not used
There was a problem hiding this comment.
That is intentional. They were not used before and I aimed to preserve the existing behavior as close as possible.
I don't know exactly why they weren't used in the first place or why this only transforms text and json but the goal of this PR was not to re-write the capability of this function; just swap out the components it uses to make building easier.
| responseHeaders[key] = value; | ||
| }); | ||
| deferred.reject( | ||
| // TODO: there is not directly equivalent to http.IncomingMessage, is the full obj ok as the second arg here? |
There was a problem hiding this comment.
I don't see a problem in RequestErrorEvent -- the second argument is simply reported
There was a problem hiding this comment.
There was a problem hiding this comment.
If something is changed for the downstream user, it should be noted in CHANGES.md.
There was a problem hiding this comment.
Thanks @jjspace! As for a testing plan, based on the original PR where NodeJS compatibility was added:
I tested using sampleTerrain using a node.js application, which is the main reason I made this change.
So the test.*js test cases are likely a good candidate for whether or not this is working as needed.
Potentially, another node script which load a 3D tileset and checks some basic properties may also be a good use case.
|
Thanks @jjspace! I'll take a final pass on this shortly.
I agree this counts as a breaking change and should be listed as such. |
|
Looks good @jjspace. I have tested the packed npm module locally as well. Everything is working as expected! |
Description
Working on updating
cesium-webpack-exampleand creatingcesium-vite-exampleI realized there are 2 main sticking points to using Cesium in various JS build environments.http,https,urlandzlib, which are only used inResource.jswhen on nodeThis PR aims to remove that second point by swapping out the "node only" logic for
fetch()which has been enabled by default since Node v18.As far as I can tell I was able to mimic the existing behavior except for one issue with an error that's thrown. There's currently no Specs set up for running in a Node environment so running the smoke screen tests
Specs/test.cjsandSpecs/test.mjswas the only way to verify and that works correctly.I'd really appreciate more eyes on this as I know
Resourceis a fairly core class to cesium and I don't want to negatively impact anything.Issue number and link
No issue
Testing plan
node Specs/test.cjsnode Specs/test.mjsAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change