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 _scriptDir for node.js + USE_PTHREADS + MODULARIZE #9875

Merged
merged 13 commits into from
Nov 27, 2019

Conversation

VirtualTim
Copy link
Collaborator

node.js doesn't have document.currentScript.src, so use __filename instead.
Since you can have a module designed for both node and the web default to document.currentScript.src, but fall back to __filename if that fails.

node.js doesn't have document.currentScript.src, so use __filename instead.
Since you can have a module designed for both node and the web default to document.currentScript.src, but fall back to __filename if that fails.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Nice fix.

Please add a test for this.

@VirtualTim
Copy link
Collaborator Author

VirtualTim commented Nov 22, 2019

I'm not really sure the best way to test this. The only node tests I can see is the one in tests/test_core.py. Perhaps it's better to make a tests/test_node.py (similar to tests/test_browser.py) and put all the node tests in there?
I have a feeling that writing a test is going to be heaps more work than implementing the fix.

@kripken
Copy link
Member

kripken commented Nov 23, 2019

Oh, tests/test_other.py is probably the best place for this. See for example test_node_js_run_from_different_directory which does something slightly related, I think?

But maybe I'm missing the difficulty in testing this - is it harder than cloning and modifying that test a little?

@VirtualTim
Copy link
Collaborator Author

OK, I think I got the test sorted.
I did have to compile with EXPORT_NAME, as it appears that 'Module' is already be defined in node.js.
I still think it's worth (one day) creating something like test-environment-node that contains node.js specific tests, but I think that should be a separate issue.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! lgtm with that one final nit.

'''
if not os.path.exists('subdir'):
os.mkdir('subdir')
with open(os.path.join('subdir', moduleLoader), 'w+', encoding='utf-8') as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a helper function for this, create_test_file(name, contents)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change, but I'm not sure what's going on with the tests. My test passed, but some other ones appear to have hung.

@kripken
Copy link
Member

kripken commented Nov 27, 2019

Yeah, weird about those tests... I can't seem to restart them.

Looks safe to land though as all the other tests passed, landing!

@kripken kripken merged commit e7ef5b2 into emscripten-core:incoming Nov 27, 2019
@VirtualTim VirtualTim deleted the patch-3 branch November 28, 2019 01:28
sbc100 added a commit that referenced this pull request Nov 28, 2019
sbc100 added a commit that referenced this pull request Nov 29, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
…re#9875)

node.js doesn't have document.currentScript.src, so use __filename instead.
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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