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

Fix parser strictMode option #13548

Merged
merged 1 commit into from Jul 20, 2021
Merged

Conversation

@overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Jul 11, 2021

Q                       A
Fixed Issues? No issue
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link n/a
Any Dependency Changes? No
License MIT

The strictMode option to parse() does not behave as described in the docs.

The docs say:

strictMode: By default, ECMAScript code is parsed as strict only if a "use strict"; directive is present or if the parsed file is an ECMAScript module. Set this option to true to always parse files in strict mode.

The actual behavior is strict = options.strictMode === false ? false : options.sourceType === "module";

Setting the option to true has no effect - contradicting what it says in docs.

This PR fixes this so the option behaves as documented, while also preserving the current (undocumented) behavior of allowing strictMode: false to override sourceType: 'module'.

I have opted to check for true and false explicitly and ignore any other values for the option, rather than accepting other truthy/falsy values. This is intended to make this change less likely to cause unexpected breakage for incorrect configurations e.g. strictMode: "sloppy". But I think anyone who has explicitly said strictMode: true clearly wants that!

By the way, this option is useful in some cases. I ran into it because was trying to parse code that was being run in an eval() which is nested within strict mode code. So it should be parsed as strict mode, but not as a module as there's no import inside eval().

@codesandbox
Copy link

@codesandbox codesandbox bot commented Jul 11, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ffe9d58:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

I don't like this option, but yeah let's at least make it behave correctly.

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

We should update TypeScript definition file for this option.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jul 20, 2021

Uh well, it was already defined as an optional boolean.

@sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Jul 20, 2021

Oh sorry I missed it

@JLHwung JLHwung merged commit dd942f9 into babel:main Jul 20, 2021
23 checks passed
@overlookmotel
Copy link
Contributor Author

@overlookmotel overlookmotel commented Jul 21, 2021

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants