-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add sourceType 'function' #6671
Comments
There is currently a |
I mention that in the bug report, but that's not what I want. I want the parser to be in the mode that the spec says new Function(value) is parsed in. Is that what allowReturnOutsideFunction enables? |
Specifically that algorithm is this: which can parse as either So you probably need 3 source types, one for each type of function. |
I don't see that in there?
Fair enough. I don't think this would be That said, is there any reason you wouldn't just actually wrap your code in a function before parsing it, then just parse it as a normal file? |
Sorry I should have been more clear, that's what I meant by "an error about return statements which we can suppress, but that's not really what we want".
Yeah that would be fine. In the same way there's parseExpression, parseFunction would be nice.
For the same reasons we no longer do that in web browsers for inline event handlers, and the same reason the spec doesn't do that. It means invalid code will be parsed as valid and things can confusingly escape from the enclosed function. That's the reason to use
Since a body like Trying prevent all of those situations leads you down a path of trying to parse the code with regexes and wrap it with progressively more complex wrappers (we tried all this in Chrome, it was a bad idea :)) The reason to use a parser like Babel is to avoid having to ever parse or interact with the code as a string and instead make sure to interact with it always as an object structure as defined by the spec. I suppose an alternative would be to add analogs to |
I'm strongly in favor of what Logan proposed in babel/babylon#341 (comment). I already have a working implementation of it since I used it a while ago in a personal project, so I can open a PR, if wanted. |
For this particular case, you can just assert on the shape of the parsed code, surely: function parseFunctionBody(src, isGenerator, isAsync) {
const fn = parseExpression('(' + (isAsync ? 'async ' : '') + 'function ' + (isGenerator ? '*' : '') + '(){' + src + '\n})');
if (fn.type !== 'FunctionExpression') {
throw new SyntaxError('error');
// you can also walk the tree to find the location of the leftmost function expression and throw an error with location pointing at its closing brace
}
return fn.body;
} I suppose this doesn't give you precise location information for syntax errors which occur after an extra Which isn't to say I'm opposed to something more in the API, but I'd like to better understand why you don't want to go with this approach. |
We don't want to go that approach because argument names are still passed in as raw strings so we'd need to validate those, plus we'd need to validate that there's only one function in the list (since you could escape and end up with two). I suppose we could parse it with no function names, pull the function body out, serialize just that code, and then pass it down through new Function. It's getting uglier and uglier though. Overall this means abandoning the .transform() abstraction and implementing a bunch of our own parsing logic which is unfortunate since these parse steps are both used by the browser for things like I'm surprised you're so opposed to adding the same API the spec has. Being able to do (Also fwiw having this API makes much more sense to me than something like |
That seems like it would still be true with or without the proposed API, no? An API for parsing a function body isn't going to help you with parameter names.
That's pretty much what I've proposed above, yes. It doesn't actually seem all that ugly to me.
Like I say, I'm not necessarily opposed. My main concern is, well, there are a lot of different contexts which one could want something parsed in - function body, expression inside or outside an async function, loop body, case body, parameter, class body, etc. I don't think all of these should be added; I'd be a lot happier with an API that satisfied all of these cases, including possibly others we haven't thought of. But when I think about what that would look like, I end up thinking that the existing APIs + wrappers like the one I've proposed above seem to already satisfy it. Wrapping your input in something which gives you the context you want and then pulling out the subtree you care about, while asserting that the full parse results in a node of the type you're expecting (so you know it didn't accidentally escape the context you wanted), seems like a fully general solution to the problem of "parse this thing in this context". With small helpers to handle serialization you'd still get all the power of (Also, despite my "member" flair, I'm largely just a bystander here, commenting only from my experience on other parsers and with the spec. Don't put too much wait on my opinions.) |
|
Closing, out of scope for now. |
We have snippets of JS supplied by content creators that should run inside a function and which we pass into new Function(transform(code)), but babel will only let us parse starting at either a top level file or module instead of inside a function. This manifests in an error about return statements which we can suppress, but that's not really what we want since we want it to parse based on the rules of new Function not invalid top level file syntax.
The text was updated successfully, but these errors were encountered: