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

More use of JS method syntax. NFC #19906

Merged
merged 1 commit into from
Aug 1, 2023
Merged

More use of JS method syntax. NFC #19906

merged 1 commit into from
Aug 1, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jul 27, 2023

No description provided.

@sbc100 sbc100 requested a review from brendandahl July 27, 2023 08:26
@sbc100 sbc100 enabled auto-merge (squash) July 27, 2023 16:10
@sbc100 sbc100 merged commit 0ac2d7d into main Aug 1, 2023
2 checks passed
@sbc100 sbc100 deleted the use_method_syntax branch August 1, 2023 01:03
@jameshu15869
Copy link
Contributor

Just to check, is the method syntax or arrow syntax preferred? It looks like there were some functions in library_wasmfs.js that were left as arrow functions (e.g. close, mkdir). Is there a specific reason that arrow syntax was used in those cases and not for the other methods? It looks like the arrow syntax methods were basically all inside FS.handleError() so should I use arrow functions for one line returns and method syntax for the others in the future?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 2, 2023

I think the answer is that we should probably prefer whichever produces the smallest code size. In almost all cases that is method syntax so we should, in general prefer that.

I think the only exception is single expression functions that return a value. In this case we can use the short-form of the arrow syntax that avoids the return keyword. If that ends up being too confusing of a rule then I think we should prefer method syntax everywhere.. especially since it avoid the this binding issue that arrow functions have.

@jameshu15869
Copy link
Contributor

Got it. Is there a measurement for code size in JS files like metadce_files_wasmfs.size? Or would it more be based on character count/lines taken up in the JS file itself?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 2, 2023

Yes, the measurement of code size here comes from the metadce and other codesize tests in the test_other.py. To see the effect of a given change on code size I normally run ./test/runner other.*code_size* other.*metadce* --rebase.

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 15, 2024
This was broken in emscripten-core#19906, but we didn't have any testing for this
particular combination of flags.
sbc100 added a commit that referenced this pull request May 15, 2024
This was broken in #19906, but we didn't have any testing for this
particular combination of flags.
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.

3 participants