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

Fix jsep #5365

Merged
merged 2 commits into from
May 25, 2017
Merged

Fix jsep #5365

merged 2 commits into from
May 25, 2017

Conversation

lilleyse
Copy link
Contributor

This is the same change as in #5358. I'm not sure how to make this library work naturally with requirejs, maybe you'll have some idea @mramato. This change though is needed in the short term because Sandcastle is broken on 3d-tiles.

@lilleyse lilleyse mentioned this pull request May 25, 2017
@mramato
Copy link
Contributor

mramato commented May 25, 2017

Compare the file in this branch to https://github.com/soney/jsep/blob/master/build/jsep.js#L679 and you may notice it was changed a bunch from the origin (not just indenting but also the surrounding IIFE was modified to use define so it would "work" with requirejs.

The problem that led to the bug you're trying to fix is that it was previously passing in the this object into the function (which in the browser case is the Window object). This is how root got defined. That no longer happens with the hacked up AMD version which leads to it trying to access the undefined root and crashing.

The solution is not to remove code but rather to go back to exactly what is related by jsep as the official build and not modify the code at all. Instead, we wrap the code in a separate define block and the original IIFE stays in place. We then return jsep.noConflict(); from the define block which initialized the jsep object and also makes sure we haven't modified global state.

I'll submit the real fix to this branch.

@mramato
Copy link
Contributor

mramato commented May 25, 2017

I'll add that someone tried to open a PR to jsep to fix this 3 years ago: EricSmekens/jsep#16 but no one ever responded to it. We can certainly open a new one that's very similar to see if they'll merge it but I would be surprised.

Unfortunately, stuff like this is just the sad state of JavaScript libraries, there's no way to use a lot of them without some modification for requirejs (and moving to ES6 probably won't fix it either). Wrapping like the change I just submitted is our standard tactic for IIFEs. The real bug here is that someone updated the file without running the test 😉

@mramato
Copy link
Contributor

mramato commented May 25, 2017

Here's an ignored whitespace diff to see what I was actually talking about in action (looks like the latest version has a couple of new functions as well): https://github.com/AnalyticalGraphicsInc/cesium/pull/5365/files?w=1

@mramato
Copy link
Contributor

mramato commented May 25, 2017

This is ready, that random requirejs issue is long standing and something I really need to fix at some point.

@lilleyse
Copy link
Contributor Author

Thanks @mramato for the explanation and fixes.

@lilleyse lilleyse merged commit 3dafb4f into 3d-tiles May 25, 2017
@lilleyse lilleyse deleted the jsep-again branch May 25, 2017 15:01
@hpinkos hpinkos mentioned this pull request Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants