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

feat(experimental-utils): move isTypeReadonly from eslint-plugin to experimental-utils #3658

Merged

Conversation

RebeccaStevens
Copy link
Contributor

@RebeccaStevens RebeccaStevens commented Jul 25, 2021

files moved:

  • isTypeReadonly
  • nullThrows
  • propertyTypes

Fixes #3657
Fixes #3305

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @RebeccaStevens!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@nx-cloud
Copy link

nx-cloud bot commented Jul 25, 2021

☁️ Nx Cloud Report

CI ran the following commands for commit edb5bc9. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 48 targets

Sent with 💌 from NxCloud.

@codecov
Copy link

codecov bot commented Jul 25, 2021

Codecov Report

Merging #3658 (edb5bc9) into main (4bb55a2) will decrease coverage by 2.00%.
The diff coverage is 30.76%.

@@            Coverage Diff             @@
##             main    #3658      +/-   ##
==========================================
- Coverage   93.88%   91.88%   -2.01%     
==========================================
  Files         297      168     -129     
  Lines       11193     8382    -2811     
  Branches     3253     2666     -587     
==========================================
- Hits        10509     7702    -2807     
- Misses        416      481      +65     
+ Partials      268      199      -69     
Flag Coverage Δ
unittest 91.88% <30.76%> (-2.01%) ⬇️

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

Impacted Files Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts 98.30% <ø> (ø)
...ackages/eslint-plugin/src/util/getWrappingFixer.ts 100.00% <ø> (ø)
packages/type-utils/src/containsAllTypesByName.ts 0.00% <0.00%> (ø)
...ges/type-utils/src/getConstrainedTypeAtLocation.ts 0.00% <0.00%> (ø)
packages/type-utils/src/getContextualType.ts 0.00% <0.00%> (ø)
packages/type-utils/src/getDeclaration.ts 0.00% <0.00%> (ø)
packages/type-utils/src/getSourceFileOfNode.ts 0.00% <0.00%> (ø)
packages/type-utils/src/getTokenAtPosition.ts 0.00% <0.00%> (ø)
packages/type-utils/src/getTypeName.ts 0.00% <0.00%> (ø)
packages/type-utils/src/isTypeReadonly.ts 0.00% <0.00%> (ø)
... and 157 more

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Hmm.. This might be a problem for some of our consumers.
Many consumers of @typescript-eslint/experimental-utils do so in environments that don't actually use typescript.

for example eslint-plugin-jest can be used by non-TS projects, but because the plugin is written in TS they use experimental-utils.

Could you please test two things for me:

(1) do a raw install of the plugin into a folder with no typescript dependency.

eg

cd /tmp
mkdir ts-eslint-test-3658
cd ts-eslint-test-3658
yarn add file:~/path/to/your/checkout/packages/experimental-utils

I believe that this should be the correct command to add a local path, and it should show warnings. I think.

The idea is that this should install fine with no warnings about missing dependencies.

We might be able to get around the warning by explicitly marking the TS dependency as optional:
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/package.json#L68-L72

(2) make sure it doesn't crash when imported from an environment without TS installed.

cd /tmp/ts-eslint-test-3658
node --eval="require('@typescript-eslint/experimental-utils')"

This should not crash (I might have got the node command wrong... also try node and then executing the require in the REPL).

This should execute without crashing.

packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts Outdated Show resolved Hide resolved
packages/experimental-utils/typings/typescript.d.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Aug 1, 2021
@RebeccaStevens
Copy link
Contributor Author

RebeccaStevens commented Aug 1, 2021

I added typescript and tsutils as optional peer dependencies and conditionally loaded them when needed so that environments without typescript don't have issues.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 2, 2021
@RebeccaStevens RebeccaStevens changed the title feat(experimental-utils): move some util function files from eslint-plugin to experimental-utils feat(experimental-utils): move isTypeReadonly from eslint-plugin to experimental-utils Aug 6, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 13, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 13, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 13, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 13, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 13, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 14, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 14, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 14, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 16, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 19, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 28, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Aug 28, 2021
@RebeccaStevens RebeccaStevens force-pushed the feat/move-isTypeReadonly branch 2 times, most recently from 66f8343 to 81dd8df Compare September 6, 2021 17:40
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Oct 24, 2021
@bradzacher
Copy link
Member

No problemo - thanks for all your work!

To make things easier for you lets see if we can get this merged sooner rather than later so we don't have to keep rebasing and dealing with merge conflicts!

I'll see if I can find some time this weekend to get this merged.

@RebeccaStevens
Copy link
Contributor Author

Sounds good.
Thanks 😄

@netlify
Copy link

netlify bot commented Nov 11, 2021

❌ Deploy Preview for typescript-eslint failed.

🔨 Explore the source changes: edb5bc9

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/61ccafa134fc990007340b8b

@bradzacher
Copy link
Member

update: I haven't forgotten about this - on my todo list. Hopefully this weekend.

@RebeccaStevens RebeccaStevens force-pushed the feat/move-isTypeReadonly branch 2 times, most recently from c9a11da to 4fc58d2 Compare November 18, 2021 00:06
@RebeccaStevens RebeccaStevens force-pushed the feat/move-isTypeReadonly branch 3 times, most recently from f665577 to 481dd62 Compare December 7, 2021 11:27
@RebeccaStevens
Copy link
Contributor Author

@bradzacher Updated again.

@bradzacher
Copy link
Member

sorry it took me so long to get back to this!
I rebased, fixed the integration tests, and moved more of our type utils across.
I'll merge this as soon as the checks pass!

bradzacher
bradzacher previously approved these changes Dec 29, 2021
bradzacher
bradzacher previously approved these changes Dec 29, 2021
@bradzacher bradzacher merged commit a9eb0b9 into typescript-eslint:main Dec 29, 2021
RebeccaStevens added a commit to eslint-functional/eslint-plugin-functional that referenced this pull request Jan 8, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2022
@RebeccaStevens RebeccaStevens deleted the feat/move-isTypeReadonly branch March 10, 2023 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
2 participants