Skip to content

remove evil new Function in $createNamedFunction #17267

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.

Sign up for GitHub

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

Closed
NickCarducci wants to merge 1 commit into from
Closed

remove evil new Function in $createNamedFunction #17267

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions src/embind/embind.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,14 +205,11 @@ var LibraryEmbind = {
return body.apply(this, arguments);
};
#else
/*jshint evil:true*/
return new Function(
"body",
"return function " + name + "() {\n" +
" \"use strict\";" +
" return body.apply(this, arguments);\n" +
"};\n"
)(body);
return {
[name](...args) {
return body(...args);
Copy link
Collaborator

@sbc100 sbc100 Jun 19, 2022

Choose a reason for hiding this comment

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

Sadly we don't yet support the spread operator in output code. Hopefully it should be available soon as we are moving towards allowing all of ES6. See #11984 for more info on ES6 support.

Copy link
Collaborator

@sbc100 sbc100 Jun 19, 2022

Choose a reason for hiding this comment

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

Do you know why this #else block exists? Why don't we just always use the simpler DYNAMIC_EXECUTION == 0 form above?

Copy link
Contributor Author

@NickCarducci NickCarducci Jun 19, 2022

Choose a reason for hiding this comment

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

I have encountered it for v8 on Cloudflare workers when it was trying to tell me the modularized Module has no default export.

Copy link
Collaborator

@sbc100 sbc100 Jun 19, 2022

Choose a reason for hiding this comment

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

But do you know why we can't just completely remove the if/else here? (and just use what is in the of block now in all cases?)

Copy link
Contributor Author

@NickCarducci NickCarducci Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Noope!

Copy link
Contributor Author

@NickCarducci NickCarducci Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I could, but it would be a favor not something I need (someone else had a reason for it, even with environment=web). I believe a better PR would be to map an array of arguments instead of[ using] the spread operator. I can do that and achieve-absolutely unlimited arguments within the week! Type Y to confirm.

Copy link
Collaborator

@sbc100 sbc100 Jun 19, 2022

Choose a reason for hiding this comment

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

I'm not quite sure what you mean by "map an array of arguments" or "absolutely unlimited arguments". Perhaps the best thing to do would be upload a proposed change.

In the mean time I might look into removing the if/else as a intermediate step.

Copy link
Collaborator

@sbc100 sbc100 Jun 19, 2022

Choose a reason for hiding this comment

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

Actually it looks like closure does a good job of transpiling these https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#computed_property_names to ES5 so I think your solution is good.

Any it works for DYNAMIC_EXECUTION so if you want to update this PR to replace the whole if/else that would work.. or I'm happy to do that too.

Copy link
Contributor Author

@NickCarducci NickCarducci Jun 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

wow
a week flys by when you are having fun and almost embarrassed
does this other pull request work? should I truly not be editing this one? I haven't done these before
{ [name]: function(arguments){ var context = {}; for (var prop in body) { context[prop] = body[prop]; }; return body.apply(context,arguments); } }[name]; (rebase/new pull request)

{
    [name]: function(arguments){
        return body.apply(this,arguments);
    }.bind(body)
}                                                [name]

For readers, export {Module as default}, maybe make ENVIRONMENT_IS_WORKER = false; with your rollup build dist (to cloudflare's v8 workers); however, I am working through aborted - (a)sync fetching of the wasm failed.. please hold if related.. (update 14d: no need to alter emcc build script outfile for v8 cloudflare service workers, the js glue code was used by them to read .wat instead of .wasm because the VSCode IDE does not allow you to see .wasm as text and the WebAssembly extension just translates the binarily encoded bytecode to .wat. So, now it is recognized by the abstract sw by js glue code and wrangler.toml\wasm_modules={ENVMODULE1="./a.out.wasm"}, and actually may need to edit some of the glue js outfile).

Copy link
Collaborator

@sbc100 sbc100 Jun 20, 2022

Choose a reason for hiding this comment

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

You can edit existing PRs, or open new ones. Its up to you.

}
}[name];
#endif
},

Expand Down