-
Notifications
You must be signed in to change notification settings - Fork 960
feat: add sourceType: commonjs option #1377
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
I don't think we should go against the ESTree spec on what |
Thank you for checking this PR! I changed |
This looks pretty good to me. The only other thing that comes to mind is it might be worth running all the |
Thank you for checking this PR! After adding tests, I noticed that when using |
It might be reasonable to raise an error when |
Thank you for the comment! I changed it to reject the combination of |
Thanks, this looks good now. |
@@ -137,6 +137,9 @@ export function getOptions(opts) { | |||
if (isArray(options.onComment)) | |||
options.onComment = pushComment(options, options.onComment) | |||
|
|||
if (options.sourceType === "commonjs" && options.allowAwaitOutsideFunction) | |||
throw new Error("Cannot use allowAwaitOutsideFunction with sourceType: commonjs") | |||
|
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.
Should we also throw if allowReturnOutsideFunction: false
is used with sourceType: "commonjs"
?
close #1376
I think it's still under discussion how to design the option, but I tried implementing it.
I think the test cases included in this PR will be useful for discussion.