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

v5.0.0 #480

Merged
merged 101 commits into from
Jan 29, 2023
Merged

v5.0.0 #480

merged 101 commits into from
Jan 29, 2023

Conversation

RebeccaStevens
Copy link
Collaborator

@RebeccaStevens RebeccaStevens commented Sep 19, 2022

Closes #32
Closes #51
Closes #150
Closes #153
Closes #384
Closes #387
Fixes: #468
Fixes: #500

Install

npm i -D eslint-plugin-functional@next

yarn add -D eslint-plugin-functional@next

@RebeccaStevens RebeccaStevens added Breaking Change This change will require a new major release. Type: Feature New features or options. labels Sep 19, 2022
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #480 (9bc0752) into main (35142ba) will decrease coverage by 3.63%.
The diff coverage is 87.34%.

@@            Coverage Diff             @@
##             main     #480      +/-   ##
==========================================
- Coverage   93.82%   90.20%   -3.63%     
==========================================
  Files          34       46      +12     
  Lines         794     1215     +421     
  Branches      201      319     +118     
==========================================
+ Hits          745     1096     +351     
- Misses         24       51      +27     
- Partials       25       68      +43     
Flag Coverage Δ
4.0.2 90.12% <87.34%> (-3.58%) ⬇️
JS 89.95% <87.02%> (-3.50%) ⬇️
latest 89.95% <87.02%> (-3.50%) ⬇️
next 89.95% <87.02%> (-3.50%) ⬇️

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

Impacted Files Coverage Δ
src/utils/conditional-imports/typescript.ts 75.00% <ø> (ø)
src/configs/off.ts 66.66% <50.00%> (+4.16%) ⬆️
src/utils/merge-configs.ts 53.84% <53.84%> (ø)
src/utils/tsutils.ts 54.54% <54.54%> (ø)
src/utils/rule.ts 75.32% <75.32%> (ø)
src/rules/type-declaration-immutability.ts 78.87% <78.87%> (ø)
src/settings/immutability.ts 79.16% <79.16%> (ø)
src/rules/readonly-type.ts 86.04% <86.04%> (ø)
src/rules/prefer-immutable-types.ts 86.88% <86.88%> (ø)
src/rules/no-conditional-statements.ts 88.73% <87.50%> (ø)
... and 40 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jonaskello
Copy link
Collaborator

We have quite many projects relying mainly on prefer-readonly-type to keep things immutable. Since it will be deprecated with 5.0 I'm mostly concerned about the migration path for these projects. My thinking was that I should had time to document a migration path to get the same level of immutability using the new rules but unfortunately I have not had time to do so yet.

Perhaps one way would be some simple comparison table that references prefer-readonly-type and the replacement.

@jonaskello
Copy link
Collaborator

jonaskello commented Oct 2, 2022

For reference the rules we use are in this repo and more specifically here regarding the prefer-readonly-type rule. The options we use are { allowLocalMutation: true, ignorePattern: "^[mM]utable" }.

EDIT: To elaborate a bit on why we have these settings. We want to keep all "application code" totally immutable and side-effect free. Since (unfortunately) almost every 3rd party package does side-effect, this means we do not have any interaction with 3rd party libraries in "application code". This means combability with 3rd party calls scenario is not a problem for us at all. We do all side-effect and 3rd party calls in a thin "imperative shell", which is a separate package where we can disable the linting.

So for us the perfect solution would be if tsc supported a switch to treat all data as immutable, something like tsc --immutable. In this mode there would be no need for readonly modifer, instead in this mode tsc would support a mut modifier for explicit mutability similar to Rust, F# and other language. However since that probably never is going to happen, the next best thing is to put readonly modifiers everywhere and instead of mut modifier, have a naming convention like leading mut in the name, hence the ignorePattern setting we use.

The reason we have allowLocalMutation set to true is purely for performance. Javascript does not have persistent data types so mutation is really needed for the implementation details in functions.

@jonaskello
Copy link
Collaborator

jonaskello commented Oct 2, 2022

I think the main job of prefer-readonly-type is basically to ensure that readonly modifier (or equivalent type) is used in all places where typescript supports them. With the new rules I guess this job would be split between other rules so perhaps that could be put in a table in order to find the migration path. This is a incomplete example of that:

Where typescript supports readonly modifier/types:

Type Typescript readonly suppport
Array ReadonlyArray<T> or readonly modifier
Tuple readonly modifier
Object readonly modifier
Set ReadonlySet<T>
Map ReadonlyMap<T>
class member readonly modifier
type member readonly modifier
interface member readonly modifier
more types? ??

Before, prefer-readonly-type would enforce all above, everywhere in the codebase, with location exceptions specified as options. Instead of options for locations, there are now multiple rules. The table below lists which new rule can enforce readonly modifier/types depending on location in the code:

Location New rule
global variable ??
local variable ??
in global type/interface decl type-declaration-immutability, todo check if there is a global/local option?
in local type/interface decl type-declaration-immutability, todo check if there is a global/local option?
function param prefer-immutable-parameter-types
function return-type ??
more locations? ??

@RebeccaStevens
Copy link
Collaborator Author

Here's how the new setup will work:

const globalVar: { foo: string } = ...; // No error as this doesn't get checked.
type MutableFoo = { foo: string }; // No error as declared as mutable
const globalVar: MutableFoo = ...; // No error
type Foo = { foo: string };  // Error as not immutable
const globalVar: Foo = ...;  // No error.

With the new rules, there is currently no way to check inline type declarations.
I guess a new rule should be added to check such declarations but it's not a rule I would use myself as I tend to alias everything.

@jonaskello
Copy link
Collaborator

I had a second look now and my understanding is that prefer-immutable-types will now enforce all readonly types that typescript offers, in all locations in the code, except for type declarations which is instead handled by type-declaration-immutableness. Would that be correct?

@RebeccaStevens
Copy link
Collaborator Author

Yeah, that's right (interfaces are also covered by type-declaration-immutableness)

@RebeccaStevens
Copy link
Collaborator Author

The formatting issue should hopefully be fixed now.

@RebeccaStevens RebeccaStevens merged commit dc93fcf into main Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment