Revert removal of createRequire
#23265
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbc100
force-pushed
the
revert_createRequire
branch
from
29fac54 to
66c5f53
Compare
|
What is the advantage? On code size it doesn't seem to help here IIUC |
|
See #23169 (comment). We have a lot of uses of |
|
I see, but if it's fixing a regression, why doesn't code size improve here? Or is the regression not covered by existing tests? |
This change effectively reverts emscripten-core#23169 because it turns out there are quite a few other places in the codebase that depend on being able to call `require`.
The regression is not covered by the existing tests. We would need to add tests that target each of our The code size regressions are pretty minor and bascially the inverse of the improvments from #23169 |
sbc100
force-pushed
the
revert_createRequire
branch
from
66c5f53 to
27c3d08
Compare
kleisauke
approved these changes
Dec 31, 2024
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since we load mostly system libraries. However for the `ws` module it is needed. This bug was being masked by the fact that we were setting `NODE_PATH` in our socket test running. This is no longer needed now that we run (non-parallel) tests in the emscripten tree (in out/test). This is followup to emscripten-core#23265 which itself was an attempt to revert 23169. Fixes: emscripten-core#23503
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since we load mostly system libraries. However for the `ws` module it is needed. This bug was being masked by the fact that we were setting `NODE_PATH` in our socket test running. This is no longer needed now that we run (non-parallel) tests in the emscripten tree (in out/test). This is followup to emscripten-core#23265 which itself was an attempt to revert 23169. Fixes: emscripten-core#23503
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since we load mostly system libraries. However for the `ws` module it is needed. This bug was being masked by the fact that we were setting `NODE_PATH` in our socket test running. This is no longer needed now that we run (non-parallel) tests in the emscripten tree (in out/test). This is followup to emscripten-core#23265 which itself was an attempt to revert 23169. Fixes: emscripten-core#23503
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since we load mostly system libraries. However for the `ws` module it is needed. This bug was being masked by the fact that we were setting `NODE_PATH` in our socket test running. This is no longer needed now that we run (non-parallel) tests in the emscripten tree (in out/test). This is followup to emscripten-core#23265 which itself was an attempt to revert 23169. Fixes: emscripten-core#23503
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since we load mostly system libraries. However for the `ws` module it is needed. This bug was being masked by the fact that we were setting `NODE_PATH` in our socket test running. This is no longer needed now that we run (non-parallel) tests in the emscripten tree (in out/test). This is followup to emscripten-core#23265 which itself was an attempt to revert 23169. Fixes: emscripten-core#23503
sbc100
added a commit
that referenced
this pull request
Jan 28, 2025
For most uses of `require()` in emscripten this does not matter since we load mostly system libraries. However for the `ws` module it is needed. This bug was being masked by the fact that we were setting `NODE_PATH` in our socket test running. This is no longer needed now that we run (non-parallel) tests in the emscripten tree (in out/test). This is followup to #23265 which itself was an attempt to revert #23169. Fixes: #23503
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change effectively reverts #23169 because it turns out there are quite a few other places in the codebase that depend on being able to call
require.