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

Future of ecmaFeatures.globalReturn #525

Open
nzakas opened this issue Nov 29, 2021 · 7 comments
Open

Future of ecmaFeatures.globalReturn #525

nzakas opened this issue Nov 29, 2021 · 7 comments

Comments

@nzakas
Copy link
Member

nzakas commented Nov 29, 2021

Problem: globalReturn can't be used with sourceType:module, and right now in ESLint we will detect this combination and set globalReturn to false. Arguably, this behavior is undesirable because ESLint is silently fixing a problem rather than letting the user know that it happened.

We recently just implemented sourceType: commonjs, which effectively makes ecmaFeatures.globalReturn obsolete, so there are several options we could pursue going forward:

  1. We could remove ecmaFeatures.globalReturn altogether. I think this is the cleanest solution now that we have sourceType: commonjs, however, we would probably want to throw an error if ecmaFeatures.globalReturn is specified to let people know that it has been removed.
  2. We could keep ecmaFeatures.globalReturn but throw an error if it is used with sourceType: module. This error would bubble up to ESLint and to the user, allowing them to fix their configuration.

In either case, we could remove the logic from ESLint completely.

Thoughts?

@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Nov 29, 2021
@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Dec 3, 2021
@btmills
Copy link
Member

btmills commented Dec 31, 2021

Option 1 seems cleanest to me as well. We'd be able to give a very clear and actionable error message: ecmaFeatures.globalReturn is gone, so you should set sourceType: "commonjs" instead.

@mdjermanovic
Copy link
Member

If we are not aware of any use cases for globalReturn = true other than commonjs modules, then option 1 seems best to me.

@nzakas
Copy link
Member Author

nzakas commented Jan 28, 2022

Marking as accepted.

@nzakas nzakas moved this from Feedback Needed to Blocked in Triage Jan 28, 2022
@amareshsm
Copy link
Member

amareshsm commented May 21, 2022

I would like to work on this. Is this ready to implement?

Just to confirm my understanding is correct the changes are

  1. Need to remove gloablReturn completely from espree
  2. Throw an error when ecmaFeatures: { globalReturn: true/false } specified in the options

@nzakas
Copy link
Member Author

nzakas commented Jun 1, 2022

We aren’t quite ready to implement this yet. We will need to wait for the new config system to roll out and give advance notice before we remove any existing functionality.

@amareshsm
Copy link
Member

Is it good to go now?

@nzakas
Copy link
Member Author

nzakas commented Dec 28, 2023

Not yet. This will need to happen in the v10.0.0 timeframe, once the old config system has been removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Blocked
Triage
Blocked
Development

No branches or pull requests

4 participants