-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix for repeated require() when MODULARIZE=1 is used
#5569
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
Conversation
tests/test_other.py
Outdated
| output = Popen([os.path.join(self.get_dir(), 'files.o.run')], stdin=open(os.path.join(self.get_dir(), 'stdin')), stdout=PIPE, stderr=PIPE).communicate() | ||
| self.assertContained('''size: 37 | ||
| data: 119,97,107,97,32,119,97,107,97,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35,35 | ||
| loop: 119 97 107 97 32 119 97 107 97 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 35 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, that whitespace actually matters. i pushed a fix to incoming now so it doesn't look unnecessary to editors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased against incoming
| assert "module['exports'] = NotModule;" in src | ||
| output = Popen(NODE_JS + ['-e', 'var m = require("./a.out.js"); m();'], stdout=PIPE, stderr=PIPE).communicate() | ||
| assert output == ('hello, world!\n', ''), 'expected output, got\n===\nSTDOUT\n%s\n===\nSTDERR\n%s\n===\n' % output | ||
| Popen([PYTHON, EMCC, path_from_root('tests', 'hello_world.c'), '-s', 'MODULARIZE=1', '-s', 'NO_EXIT_RUNTIME=1']).communicate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment saying this tests two calls to require
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added
|
|
||
| Module['arguments'] = process['argv'].slice(2); | ||
|
|
||
| #if MODULARIZE == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment saying why this is not needed in that mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added
|
I met this issue as well, and my solution to it was not to use var module = undefined;
// or let module = undefined; // works under no optimisation, but under optimisation this failsIn fact this was the only way to use ES6 modules. The |
src/shell.js
Outdated
|
|
||
| Module['arguments'] = process['argv'].slice(2); | ||
|
|
||
| // With MODULARIZE == 1 wrapper function will be exported instead of Module itself and re-exporting it second time will cause issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i still don't fully understand this even with the comment, sorry.
why is the second time special? and why does it matter we export the wrapper function and not the Module? I don't see a connection here that is important, I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this patch var m = require('module_name') initially exports wrapper function module.exports = wrapper_function, and we need to call this wrapper function to obtain Module itself.
However, when m() is called it re-exports module.exports = Module.
Thus successive require('module_name') doesn't return wrapper function anymore, but instead returns plain Module.
Just try to run added test on current incoming branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two Module variables. When you require it the first time you get the outer wrapper Module, and the second time you get the inner true Module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, so there are two separate places we export, one inside the module - in shell.js, which is disabled by this PR - and another outside the module, added by MODULARIZE, and is the proper place?
If that's correct then looks good to me. Let's add that to the comment, though, something like MODULARIZE will export the module in the proper place outside, we don't need to export here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct, updated comment accordingly
|
Great, thanks. |
Fixes #5568