-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Rename JS library files from library_foo.js to libfoo.js #23348
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
Merged
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
Marking as draft for now. |
64743d3
to
49a1a75
Compare
kripken
reviewed
Jan 17, 2025
kripken
reviewed
Jan 17, 2025
49a1a75
to
31a7e36
Compare
5a3327c
to
cf00d64
Compare
sbc100
added a commit
that referenced
this pull request
Jan 17, 2025
Files references via #include can now either relative to the including file or come from the system library. Previously only relative paths were allowed. Also we now give a nicer error message when the file is not found. Split out from #23348
cf00d64
to
a24778e
Compare
Much cleaner PR now that #23449 landed |
kripken
approved these changes
Jan 17, 2025
At the same time, move all the library file into a separate `src/lib` directory. This helps distinguish library files from other files in the `lib` directory, which is currently very overcrowded/overloaded. These files were also includ-able via `-lfoo` and we previously had to use a special case for system libraries where `library_foo.js` could be used in addition to more standed `libfoo.js`. This change removes to special case. This only file that was not renamed using the simple pattern was `library.py` itself which becomes `libcore.py`. I also considered `libemscripten.js`. To some this may seem like needless churn, but for years now the `src` directory (IMHO) has been asking for cleanup. I think having files logically separated into directories helps a lot with understanding the project structure for both newcomers and old timers alike. As a followup, I am also considering moving all the runtime files into `src/include` (since these are all files that get `#included` into the generated code directly. That would leave the `src` directory containing only the JS compiler code itself which I think is another important distinction.
a24778e
to
e839ead
Compare
Added a ChangeLog entry |
lgtm |
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Feb 7, 2025
This was broken when I move the library files in emscripten-core#23348 This was only used for generating the symbol lists, so the only side effect was the the symbol lists would remain stale if somebody was modifying the system libraries (i.e. it only effected emscripten developers, not users).
sbc100
added a commit
that referenced
this pull request
Feb 7, 2025
This was broken when I move the library files in #23348 This was only used for generating the symbol lists, so the only side effect was the the symbol lists would remain stale if somebody was modifying the system libraries (i.e. it only effected emscripten developers, not users).
This was referenced Feb 19, 2025
sbc100
pushed a commit
that referenced
this pull request
Jun 14, 2025
This fixes an issue where `path.resolve()` resolves using the CWD (which is usually the target project path) instead of the directory of the script itself. The current behavior [causes issues with the Closure compiler](godotengine/godot#107460 (comment)), as stubs are erroneously created instead of being skipped in `src/shell.js`. Closure then complains that variables are declared twice. The issue was caused by #23348 which changed the logic of the search.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
At the same time, move all the library file into a separate
src/lib
directory. This helps distinguish library files from other files
in the
lib
directory, which is currently very overcrowded/overloaded.These files were also includ-able via
-lfoo
and we previously had touse a special case for system libraries where
library_foo.js
could beused in addition to more standed
libfoo.js
. This change removes tospecial case.
This only file that was not renamed using the simple pattern was
library.py
itself which becomeslibcore.py
. I also consideredlibemscripten.js
.To some this may seem like needless churn, but for years now the
src
directory (IMHO) has been asking for cleanup. I think having files
logically separated into directories helps a lot with understanding the
project structure for both newcomers and old timers alike.
As a followup, I am also considering moving all the runtime files into
src/include
(since these are all files that get#included
into thegenerated code directly. That would leave the
src
directorycontaining only the JS compiler code itself which I think is another
important distinction.