feat: `no-implicit-globals` supports `exported` block comment by sosukesuzuki · Pull Request #16343 · eslint/eslint

sosukesuzuki · 2022-09-23T18:13:29Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixes #16327

Variables specified with /*exported variableName */ should not report errors by no-implicit-globals.

Is there anything you'd like reviewers to focus on?

#16343 (comment)

eslint-github-bot · 2022-09-23T18:13:34Z

Hi @sosukesuzuki!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

netlify · 2022-09-23T18:13:41Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`0c32702`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/634812c6fbbe0b0008bacaa6
😎 Deploy Preview	https://deploy-preview-16343--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

eslint-github-bot · 2022-09-23T18:17:51Z

Hi @sosukesuzuki!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

sosukesuzuki · 2022-09-23T18:25:36Z


        if (variable) {
            variable.eslintUsed = true;
+            variable.eslintExported = true;


Here is a new property variable.eslintExported.
This represents the variable specified by the /* exported variableName */ comment (It represents export only unlike eslintUsed).

I'm not sure if this is a good. Is there a better way?

I think this is the right approach. variable.eslintUsed could have been set to true by another rule via context.markVariableAsUsed(), so it's not a reliable indicator that the variable is intended to be used by other scripts, which is what we need in this rule and what /* exported */ means.

But, this is a change in the core so I'd like more opinions from @eslint/eslint-tsc

Makes sense to me.

sosukesuzuki · 2022-10-07T01:16:15Z

Is there anything I should do to make progress on this?

nzakas

LGTM. Just one suggestion to clean things up a bit.

nzakas · 2022-10-12T19:31:45Z

+                    // Variables exported by "exported" block comments
+                    const isExportedEslintVariables = variable.eslintExported;
+
+                    if (isExportedEslintVariables) {


We don’t need a variable here.

Suggested change

if (isExportedEslintVariables) {

if (variable.eslintExported) {

done! 42ac8ae

mdjermanovic · 2022-10-13T07:41:56Z

+
+var global_var = 42;
+```
+


Suggested change

:::

To close ::: correct

done! cdb5e89

mdjermanovic · 2022-10-13T08:07:34Z

+            code: "/* exported foo */ var foo = function *foo() {};",
+            parserOptions: { ecmaVersion: 2015 }
+        },
+        "/* exported foo \n* exported bar */ var foo = 1, bar = 2;",


Suggested change

"/* exported foo \n* exported bar */ var foo = 1, bar = 2;",

"/* exported foo, bar */ var foo = 1, bar = 2;",

This format is not supported. This directive would be parsed without errors, but the variables it declares as exported actually are: ["foo", "*", "exported", "bar"].

(the same for other tests with multiple exported variables)

done! c7b6c6a

mdjermanovic · 2022-10-13T08:23:07Z

+        //------------------------------------------------------------------------------
+        // exported
+        //------------------------------------------------------------------------------
+


Can you add a test to confirm that exported does not allow variables that are implicitly created by assignments (global variable leaks). For example, /* exported foo */ foo = 1; should still be invalid for this rule.

done! 0c32702

test: add valid tests test: add invalid tests

docs: add correct example

mdjermanovic

LGTM, thanks!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.25.0` -> `8.26.0`](https://renovatebot.com/diffs/npm/eslint/8.25.0/8.26.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.26.0`](https://github.com/eslint/eslint/releases/tag/v8.26.0) [Compare Source](eslint/eslint@v8.25.0...v8.26.0) #### Features - [`4715787`](eslint/eslint@4715787) feat: check `Object.create()` in getter-return ([#16420](eslint/eslint#16420)) (Yuki Hirasawa) - [`28d1902`](eslint/eslint@28d1902) feat: `no-implicit-globals` supports `exported` block comment ([#16343](eslint/eslint#16343)) (Sosuke Suzuki) - [`e940be7`](eslint/eslint@e940be7) feat: Use ESLINT_USE_FLAT_CONFIG environment variable for flat config ([#16356](eslint/eslint#16356)) (Tomer Aberbach) - [`dd0c58f`](eslint/eslint@dd0c58f) feat: Swap out Globby for custom globbing solution. ([#16369](eslint/eslint#16369)) (Nicholas C. Zakas) #### Bug Fixes - [`df77409`](eslint/eslint@df77409) fix: use `baseConfig` constructor option in FlatESLint ([#16432](eslint/eslint#16432)) (Milos Djermanovic) - [`33668ee`](eslint/eslint@33668ee) fix: Ensure that glob patterns are matched correctly. ([#16449](eslint/eslint#16449)) (Nicholas C. Zakas) - [`740b208`](eslint/eslint@740b208) fix: ignore messages without a `ruleId` in `getRulesMetaForResults` ([#16409](eslint/eslint#16409)) (Francesco Trotta) - [`8f9759e`](eslint/eslint@8f9759e) fix: `--ignore-pattern` in flat config mode should be relative to `cwd` ([#16425](eslint/eslint#16425)) (Milos Djermanovic) - [`325ad37`](eslint/eslint@325ad37) fix: make `getRulesMetaForResults` return a plain object in trivial case ([#16438](eslint/eslint#16438)) (Francesco Trotta) - [`a2810bc`](eslint/eslint@a2810bc) fix: Ensure that directories can be unignored. ([#16436](eslint/eslint#16436)) (Nicholas C. Zakas) - [`35916ad`](eslint/eslint@35916ad) fix: Ensure unignore and reignore work correctly in flat config. ([#16422](eslint/eslint#16422)) (Nicholas C. Zakas) #### Documentation - [`651649b`](eslint/eslint@651649b) docs: Core concepts page ([#16399](eslint/eslint#16399)) (Ben Perlmutter) - [`631cf72`](eslint/eslint@631cf72) docs: note --ignore-path not supported with flat config ([#16434](eslint/eslint#16434)) (Andy Edwards) - [`1692840`](eslint/eslint@1692840) docs: fix syntax in examples for new config files ([#16427](eslint/eslint#16427)) (Milos Djermanovic) - [`d336cfc`](eslint/eslint@d336cfc) docs: Document extending plugin with new config ([#16394](eslint/eslint#16394)) (Ben Perlmutter) #### Chores - [`e917a9a`](eslint/eslint@e917a9a) ci: add node v19 ([#16443](eslint/eslint#16443)) (Koichi ITO) - [`4b70b91`](eslint/eslint@4b70b91) chore: Add VS Code issues link ([#16423](eslint/eslint#16423)) (Nicholas C. Zakas) - [`232d291`](eslint/eslint@232d291) chore: suppress a Node.js deprecation warning ([#16398](eslint/eslint#16398)) (Koichi ITO) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1599 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

eslint-github-bot Bot added the triage An ESLint team member will look at this issue soon label Sep 23, 2022

sosukesuzuki force-pushed the fix-16327 branch from c9ae910 to 313a17b Compare September 23, 2022 18:17

sosukesuzuki changed the title ~~Rule Change: no-implicit-globals supports exported block comment~~ feat: no-implicit-globals supports exported block comment Sep 23, 2022

eslint-github-bot Bot added the feature This change adds a new feature to ESLint label Sep 23, 2022

sosukesuzuki commented Sep 23, 2022

View reviewed changes

sosukesuzuki marked this pull request as ready for review September 23, 2022 18:27

mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 23, 2022

mdjermanovic mentioned this pull request Oct 3, 2022

Rule Change: no-implicit-globals should not report exported #16327

Closed

nzakas approved these changes Oct 12, 2022

View reviewed changes

mdjermanovic requested changes Oct 13, 2022

View reviewed changes

Sosuke Suzuki added 6 commits October 13, 2022 22:16

feat: support "exported" block comments

c5d8233

test: add valid tests test: add invalid tests

docs: mention about exported comment

32e5d2e

docs: add correct example

chore: use variable.eslintExported directly

42ac8ae

docs: add ::: to close correct example

cdb5e89

fix: fix exported comment for multiple variables

c7b6c6a

fix: add more invalid tests for global variable leaks

0c32702

sosukesuzuki force-pushed the fix-16327 branch from 313a17b to 0c32702 Compare October 13, 2022 13:29

mdjermanovic approved these changes Oct 14, 2022

View reviewed changes

mdjermanovic added the contributor pool label Oct 14, 2022

mdjermanovic merged commit 28d1902 into eslint:main Oct 14, 2022

sosukesuzuki deleted the fix-16327 branch October 14, 2022 15:52

eslint-github-bot Bot locked and limited conversation to collaborators Apr 13, 2023

eslint-github-bot Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 13, 2023

	if (isExportedEslintVariables) {
	if (variable.eslintExported) {

	"/* exported foo \n* exported bar */ var foo = 1, bar = 2;",
	"/* exported foo, bar */ var foo = 1, bar = 2;",

Uh oh!

Conversation

sosukesuzuki commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

Uh oh!

eslint-github-bot Bot commented Sep 23, 2022

Uh oh!

netlify Bot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-eslint ready!

Uh oh!

eslint-github-bot Bot commented Sep 23, 2022

Uh oh!

sosukesuzuki Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sosukesuzuki commented Oct 7, 2022

Uh oh!

nzakas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjermanovic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sosukesuzuki commented Sep 23, 2022 •

edited

Loading

netlify Bot commented Sep 23, 2022 •

edited

Loading

sosukesuzuki Sep 23, 2022 •

edited

Loading