fix(eslint-plugin): properly coerce all types to string in `getStaticMemberAccessValue` #10004

abrahamguo · 2024-09-17T02:01:51Z

PR Checklist

Addresses an existing open issue: fixes Bug: [prefer-promise-reject-errors (and other rules too)] Cannot convert Symbol to string (8.6.0 regression) #9999
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

#9836 had some bugs with how various data types are coerced to strings when used in a computed MemberAccess; this PR fixes those bugs.

typescript-eslint · 2024-09-17T02:01:55Z

Thanks for the PR, @abrahamguo!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-09-17T02:02:07Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`731412b`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66f0a15078ae0f00082fe3ed
😎 Deploy Preview	https://deploy-preview-10004--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 4 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

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

nx-cloud · 2024-09-17T02:02:52Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 731412b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

packages/eslint-plugin/src/util/misc.ts

JoshuaKGoldberg

abrahamguo commented 5 hours ago:

... I will plan on opening a PR with the fix in the next 12 hours ...

You work fast! 😄

...and, thanks for this! We can ship a quick patch release to get this to users.

packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts

packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts

packages/eslint-plugin/src/util/misc.ts

codecov · 2024-09-17T02:29:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.70%. Comparing base (7b149b9) to head (731412b).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10004   +/-   ##
=======================================
  Coverage   88.69%   88.70%           
=======================================
  Files         426      426           
  Lines       14829    14835    +6     
  Branches     4313     4312    -1     
=======================================
+ Hits        13153    13159    +6     
  Misses       1533     1533           
  Partials      143      143

Flag	Coverage Δ
unittest	`88.70% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...t-plugin/src/rules/class-literal-property-style.ts	`100.00% <100.00%> (ø)`
.../eslint-plugin/src/rules/class-methods-use-this.ts	`93.47% <100.00%> (+0.14%)`	⬆️
...-plugin/src/util/isArrayMethodCallWithPredicate.ts	`100.00% <100.00%> (ø)`
packages/eslint-plugin/src/util/misc.ts	`100.00% <100.00%> (ø)`

abrahamguo · 2024-09-17T03:03:23Z

Lint failed due to CI flake.

packages/eslint-plugin/src/util/misc.ts

kirkwaiblinger

Some syntax changes requested, but I think the logic is good

packages/eslint-plugin/src/util/misc.ts

packages/eslint-plugin/src/util/isArrayMethodCallWithPredicate.ts

packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts

packages/eslint-plugin/src/util/misc.ts

kirkwaiblinger

I think this looks good! Thanks!

kirkwaiblinger · 2024-09-20T21:39:28Z

packages/eslint-plugin/src/util/misc.ts

-      ? String(key.value)
-      : (key as TSESTree.Identifier | TSESTree.PrivateIdentifier).name;
+  const { type } = key;
+  if (type === AST_NODE_TYPES.Literal) {


(question, no specific action/resolution implied) is this conditional necessary? Or will it just be caught by the getStaticValue?

checked locally and all the tests pass if I comment it out 🤷

good catch, removed!

…et-static-key-value

properly handle everything

8a5cb0d

reformat comment

0c863c1

update comment

d88ad41

abrahamguo commented Sep 17, 2024

View reviewed changes

packages/eslint-plugin/src/util/misc.ts Show resolved Hide resolved

abrahamguo commented Sep 17, 2024

View reviewed changes

packages/eslint-plugin/src/util/misc.ts Show resolved Hide resolved

JoshuaKGoldberg previously approved these changes Sep 17, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/prefer-promise-reject-errors.test.ts Show resolved Hide resolved

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 17, 2024

abrahamguo commented Sep 17, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/explicit-module-boundary-types.test.ts Show resolved Hide resolved

add a test more similar to the original issue

60df625

abrahamguo dismissed JoshuaKGoldberg’s stale review via 60df625 September 17, 2024 02:11

abrahamguo changed the title ~~fix(eslint-plugin): [getStaticMemberAccessValue] properly coerce all types to string~~ fix(eslint-plugin): [getStaticMemberAccessValue] properly coerce all types to string Sep 17, 2024

abrahamguo changed the title ~~fix(eslint-plugin): [getStaticMemberAccessValue] properly coerce all types to string~~ fix(eslint-plugin): properly coerce all types to string in getStaticMemberAccessValue Sep 17, 2024

abrahamguo commented Sep 17, 2024

View reviewed changes

packages/eslint-plugin/src/util/misc.ts Show resolved Hide resolved

kirkwaiblinger reviewed Sep 17, 2024

View reviewed changes

packages/eslint-plugin/src/util/misc.ts Outdated Show resolved Hide resolved

be more correct, less practical, with symbols

f9c243d

abrahamguo requested review from kirkwaiblinger and JoshuaKGoldberg September 17, 2024 12:21

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 17, 2024

kirkwaiblinger requested changes Sep 17, 2024

View reviewed changes

abrahamguo added 2 commits September 17, 2024 12:33

add jsdoc

9338c16

clean up a couple things

8abca66

kirkwaiblinger reviewed Sep 17, 2024

View reviewed changes

packages/eslint-plugin/src/util/misc.ts Outdated Show resolved Hide resolved

abrahamguo added 3 commits September 17, 2024 17:25

refactor to not use type assertion

2f1b690

fix comment

33bf910

lint

515ebbf

abrahamguo requested a review from kirkwaiblinger September 17, 2024 23:19

kirkwaiblinger previously approved these changes Sep 20, 2024

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 20, 2024

kirkwaiblinger requested a review from a team September 20, 2024 22:36

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 20, 2024

remove unnecessary check

54ababe

abrahamguo dismissed kirkwaiblinger’s stale review via 54ababe September 21, 2024 06:55

Merge branch 'main' of github.com:abrahamguo/typescript-eslint into g…

3a0d263

…et-static-key-value

abrahamguo requested a review from kirkwaiblinger September 21, 2024 06:56

kirkwaiblinger approved these changes Sep 21, 2024

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Sep 21, 2024

Josh-Cena approved these changes Sep 22, 2024

View reviewed changes

Merge branch 'main' into get-static-key-value

731412b

Josh-Cena merged commit 1c183ab into typescript-eslint:main Sep 22, 2024
61 of 62 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): properly coerce all types to string in `getStaticMemberAccessValue` #10004

fix(eslint-plugin): properly coerce all types to string in `getStaticMemberAccessValue` #10004

abrahamguo commented Sep 17, 2024

typescript-eslint bot commented Sep 17, 2024

netlify bot commented Sep 17, 2024 •

edited

Loading

nx-cloud bot commented Sep 17, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

codecov bot commented Sep 17, 2024 •

edited

Loading

abrahamguo commented Sep 17, 2024

kirkwaiblinger left a comment

kirkwaiblinger left a comment

kirkwaiblinger Sep 20, 2024

kirkwaiblinger Sep 20, 2024

abrahamguo Sep 21, 2024

fix(eslint-plugin): properly coerce all types to string in getStaticMemberAccessValue #10004

fix(eslint-plugin): properly coerce all types to string in getStaticMemberAccessValue #10004

Conversation

abrahamguo commented Sep 17, 2024

PR Checklist

Overview

typescript-eslint bot commented Sep 17, 2024

netlify bot commented Sep 17, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Sep 17, 2024 • edited Loading

☁️ Nx Cloud Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

abrahamguo commented Sep 17, 2024

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger Sep 20, 2024

Choose a reason for hiding this comment

kirkwaiblinger Sep 20, 2024

Choose a reason for hiding this comment

abrahamguo Sep 21, 2024

Choose a reason for hiding this comment

fix(eslint-plugin): properly coerce all types to string in `getStaticMemberAccessValue` #10004

fix(eslint-plugin): properly coerce all types to string in `getStaticMemberAccessValue` #10004

netlify bot commented Sep 17, 2024 •

edited

Loading

nx-cloud bot commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading