-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
Conversation
in favor of immediately identified function key object {n:fn()}[n]
| )(body); | ||
| return { | ||
| [name](...args) { | ||
| return body(...args); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noope!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
in favor of immediately identified function key object {n:fn()}[n]