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

Check whether scriptDir is defined. #12832

Closed
wants to merge 3 commits into from

Conversation

annxingyuan
Copy link

Adding the type check avoids triggering a ReferenceError if _scriptDir has not been defined.

@welcome
Copy link

welcome bot commented Nov 19, 2020

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

@kripken
Copy link
Member

kripken commented Nov 19, 2020

I may be missing something, but I'm not sure how it could be undefined. A program with MODULARIZE should start with a definition

  var _scriptDir = typeof document !== 'undefined' && document.currentScript ? document.currentScript.src : undefined;

So it could have the value undefined but it is defined, and should not error? Perhaps are you building with some combination of settings that leads to that definition not being emitted somehow?

@annxingyuan
Copy link
Author

Thanks @kripken - maybe I'm not building this properly. In TF.js, we bundle the emscripten generated JS module along with the rest of our JS. Therefore we need to pass the emscripten JS module as a Blob to the emscripten generated worker (via mainScriptUrlOrBlob), because the main script contains TF.js code that the worker doesn't understand.

The definition of _scriptDir is emitted - but we stringify the emscripten module after it has self-executed (https://github.com/tensorflow/tfjs/blob/master/tfjs-backend-wasm/src/backend_wasm.ts#L274), so the definition is not included. You can see that I define _scriptDir as part of the prefix, but this creates issues with minification.

Sorry if I'm missing an obvious alternative!

@kripken
Copy link
Member

kripken commented Nov 20, 2020

To make sure I understand: You are building with MODULARIZE, and then creating an instance from the factory function, and then stringifying that instance, then sending it somewhere, and then when it is evalled there, it is missing _scriptDir because that was defined in the factory function, which would normally be seen in the outer scope, but not after stringifying? And, the minification problem is that if you prepend var _scriptDir; to the stringified text then it won't match the minified name of _scriptDir if the emscripten output was minified?

If I got that right, then a workaround for this could be to use a --pre-js to add var _scriptDir;. That would be added inside the emscripten output, and minified all together.

However, in the bigger picture, I'm confused as to why the stringification is needed. I hope that can be avoided, as it should be simpler and more efficient? But I'm probably not understanding something. And specifically I'm not sure what you mean by

the main script contains TF.js code that the worker doesn't understand.

?

@annxingyuan
Copy link
Author

@kripken Thanks for the tip about --pre-js - I should have read the docs more carefully!

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