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

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-ci
Copy link

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

@nicolo-ribaudo nicolo-ribaudo added pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Jul 19, 2021
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

We should update TypeScript definition file for this option.

@nicolo-ribaudo
Copy link
Member

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

@sosukesuzuki
Copy link
Member

Oh sorry I missed it

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

Thanks for merging!

@overlookmotel overlookmotel deleted the parser-strictMode-option branch July 21, 2021 11:31
nicolo-ribaudo pushed a commit to nicolo-ribaudo/babel that referenced this pull request Jul 30, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants