jsep is undefined fix

[ggetz]

Fixes #5593

Fixes the jsep is undefined error that was occurring when using webpack. Tested using the configuration @lucastheisen provided in #5593

@lilleyse Can you review please?

[pjcozzi]

@ggetz please update CHANGES.md.

@lucastheisen can you confirm that this fixes your issue?

[lilleyse]

This fixes the Node issues I was having. 👍

[ggetz]

Updated CHANGES.md

[lucastheisen]

My test would be using the example that @ggetz mentioned she confirmed this fix with. However, I would be happy to also test against my real project if there is a relatively straightforward path to doing so... Do you guys have a CI build that would allow me to point to this fix? Or do I need to build from from the pull request source branch and build myself? I'll do the legwork, just curious as to the best approach...

[lucastheisen]

As far as I can tell, the jsep stuff gets embedded into the cesium js files during the build (in the build folder there is no jsep under ThirdParty). This means i cant just swap out a file to test, so I need to build. However, my corporate environment is a major pain in the neck... the npm install fails due to our wonky ssl inspection stuff... I'll keep trying, and let you know if i am able to get this going.

[lucastheisen]

Okay, after an RTFM moment, I checked out the @ggetz branch, then ran npm install; npm run release. Next I updated my package.json to use a local dependency, and was able to verify that I no longer get a jsep error. That said, now i get this one again:

18:41:46.831 Error: Cesium missing ThirdParty/pako_inflate 1 app.js:4383:27
	main http://localhost:8080/app.js:4383:27
	callDep http://localhost:8080/app.js:4245:13
	main http://localhost:8080/app.js:4378:31
	req http://localhost:8080/app.js:4452:13
	<anonymous> http://localhost:8080/app.js:228984:1
	<anonymous> http://localhost:8080/app.js:4069:2
	<anonymous> http://localhost:8080/app.js:4047:29
	__webpack_require__ http://localhost:8080/app.js:20:12
	<anonymous> http://localhost:8080/app.js:3844:1
	__webpack_require__ http://localhost:8080/app.js:20:12
	<anonymous> http://localhost:8080/app.js:234662:18
	__webpack_require__ http://localhost:8080/app.js:20:12
	<anonymous> http://localhost:8080/app.js:66:18
	<anonymous> http://localhost:8080/app.js:1:11

Which appears to have reared its ugly head again in 1.34. Guess I will wait for a patch for that and try again...

Thanks for getting this one sorted out!

[hpinkos]

We should avoid directly modifying third party files. It can be a pain when we go to update that file in the future. @mramato do you have any suggestions for working around this in a different way?

[ggetz]

Thanks for testing this out @lucastheisen, hopefully we can get #5417 sorted soon.

@hpinkos I beleive @lilleyse said we already have modified this file, if that changes anything at all.

[hpinkos]

See this PR: #5365
I'm fairly certain that stripping out this code is not the right solution here based on the changes @mramato made in that PR

[ggetz]

Would a better fix maybe be to define jsep.noConflict for node environments as well?

[mramato]

I think the correct fix is to put all of the old code back and only call jsep.noConflict() at the end if (typeof jsep.noConflict === 'function)

[ggetz]

Updated

[lilleyse]

The recent changes don't work from the Node side because jsep itself is undefined.

$ node -e "var test = require('gltf-pipeline');"
C:\Users\slilley\Desktop\asdf\node_modules\cesium\Source\ThirdParty\jsep.js:683
    if (typeof jsep.noConflict === 'function') {
               ^

ReferenceError: jsep is not defined
    at C:\Users\slilley\Desktop\asdf\node_modules\cesium\Source\ThirdParty\jsep.
js:683:16
    at Object.execCb (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r
.js:1946:33)
    at Object.check (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r.
js:1133:51)
    at Object.enable (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r
.js:1426:22)
    at Object.init (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r.j
s:1038:26)
    at callGetModule (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r
.js:1453:63)
    at Object.completeLoad (C:\Users\slilley\Desktop\asdf\node_modules\requirejs
\bin\r.js:1840:21)
    at Function.req.load (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\b
in\r.js:2613:17)
    at Object.load (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r.j
s:1935:21)
    at Object.load (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r.j
s:1084:29)
    at Object.fetch (C:\Users\slilley\Desktop\asdf\node_modules\requirejs\bin\r.
js:1074:66)

[ggetz]

@mramato Is exports.parse = jsep; supposed to be exports.jsep = jsep?

[mramato]

@mramato Is exports.parse = jsep; supposed to be exports.jsep?

That branch should never be executed the exports = module.exports = jsep; is what should be getting executed.

@lilleyse what do I need to do to reproduce this in gltf-pipeline?

[lilleyse]

I am preparing a gltf-pipeline release with npm pack npm install --production <tarball> and calling node -e "var test = require('gltf-pipeline');"

[mramato]

Thanks, I can reproduce without the pack and install parts by just requiring in gltf-pipeline.

I see what the problem is:

1. jsep gets declared as a local in the IIFE
2. In the browser, this also becomes a global, and noConflict removes the global.
3. In Node, the global is never created so jsep is undefined, leading to an except even checking for its existence.

[mramato]

Change this to if (typeof jsep !== 'undefined') { and remove the last return statement on line 687. I think that will finally do it. Add a comment to the if that mentions that jsep only exists when running in a browser, not Node.

[mramato]

What's even weirder is that in some cases, jsep is defined, even in node, and has the noConflict function. I think there's some odd side-effects going on here with loading jsep via requirejs in Node and that's the root cause of the issue. I think the above suggested changes should resolve things on the Node side for now (not sure about webpack).

When we to ES6, we'll have to revisit our strategy for how we wrap/use third party libraries.

[mramato]

Doc is failing, which is why travis is failing, but it doesn't appear to be related to this branch. @lilleyse can you test this out. Thanks.

[ggetz]

I tested with webpack and it works. I'm not sure why the build is failing on makeZipFile though, the command runs without error locally.

[lilleyse]

The latest changes work for me.

[mramato]

Thanks @ggetz!