feat!: flat config + v9 support, rule unification, add TS config #14

jirimoravcik · 2024-10-11T13:28:46Z

This PR begins a new era of linting at Apify.
It introduces a new shared eslint config. It uses the flat eslint format and works with eslint v9.

I consolidated the TypeScript config into this one, so the maintenance and updates are easier. This means we won't need apify-eslint-config-ts anymore.

I have tried to consolidate most of the rules from configurations we have available across our projects. If you still feel something should be changed, feel free to comment here.

Newly added/enabled rules:
no-use-before-define
no-param-reassign
consistent-return
array-callback-return
no-promise-executor-return
@typescript-eslint/array-type
@typescript-eslint/adjacent-overload-signatures
@typescript-eslint/no-for-in-array
@typescript-eslint/no-implied-eval
@typescript-eslint/no-inferrable-types
@typescript-eslint/no-confusing-non-null-assertion
@typescript-eslint/prefer-includes
import/no-default-export

Let's first review this PR, then I'll release a beta version that can be easily tested 😉 .
After that, we'll do final touches and merge this PR. After that, we'll have to update the config across our projects. I will oversee this and create/plan tasks in GitHub for it.

metalwarrior665 · 2024-10-31T15:35:40Z

index.js

+            // Require methods and fucnctions to be marked async if they return a Promise.
+            '@typescript-eslint/promise-function-async': 'error',
+            // Force the usage of "import type" for type imports.
+            '@typescript-eslint/consistent-type-imports': 'error',


Seems this defaults to fixStyle: 'separate-type-imports' which would error on import { type CheerioCrawlingContext } if I'm reading it right? We use that syntax in some places but I'm ok to be overturned :)

https://typescript-eslint.io/rules/consistent-type-imports/#fixstyle

I hate import { type it feels messy and ends up having several types mixed between the imports as the file grows. Also apparently this is better performance 🤔

I don't care, but having only one type seems good. I guess this can also be autofixed.

We use import { type quite extensively, because we have some other rule set up in a way that we can only have one import per file/dependency, but if the fixer works, I really don't mind.

I recall those fixers tend to screw up with whitespace, but maybe it was a different rule than this one.

fnesveda

Very cool! Just a few small points.

package.json

index.js

Co-authored-by: František Nesveda <[email protected]>

index.js

stetizu1 · 2024-11-06T10:07:15Z

index.js

+            // Require methods and fucnctions to be marked async if they return a Promise.
+            '@typescript-eslint/promise-function-async': 'error',
+            // Force the usage of "import type" for type imports.
+            '@typescript-eslint/consistent-type-imports': 'error',


I don't care, but having only one type seems good. I guess this can also be autofixed.

B4nan

sorry for the late review, way too many adhoc things

package.json

.github/workflows/publish.yaml

B4nan · 2024-11-06T14:55:02Z

.github/workflows/publish.yaml

+          git_tag=v`node -p "require('./package.json').version"`
+          git tag $git_tag
+          git push origin $git_tag


why not use npm version instead which will handle both git tag and bumping? this feels like you only deal with publishing and once released, things will fall apart unless someone bumps manually.

note that npm version should be used before npm publish

the bumping should be done manually, yes.

I really dislike that, the new release process we are adopting these days don't suffer from this unnecessary manual step, why should we deal with this manually here? Basically all our public repos (except CLI which is in progress) don't requite this now, we release via workflow dispatch and bump based on conventional commits with possible manual override. We could adopt the same in here if you want, I'd let @janbuchar to make it work since its all his work. It also means automated changelogs and GH releases and other goodies.

Ok, nice. Please send me some PR/commit that implements it, I'll do it here. Thanks!

One from the CLI, we have also canary builds in there, I don't know what we want from this repo, we could as well just ship to stable like we do in apify-shared-js, but if we want beta versions and stable releases via workflow dispatch, that's what the CLI and client are doing.

apify/apify-cli#691

In client we had two PRs since we missed one thing

apify/apify-client-js#596
apify/apify-client-js#605

index.js

B4nan · 2024-11-06T15:08:42Z

index.js

+                groups: ['builtin', 'external', ['parent', 'sibling'], 'index', 'object'],
+                alphabetize: {
+                    order: 'asc',
+                    caseInsensitive: true,
+                },
+                'newlines-between': 'always',


I am personally a bit allergic to this mixed quoting, but I guess it's better for not producing unnecessary git diffs...

(this is not about the rules highlighted, but about the quoting on the keys)

I don't follow

alphabetize is not quoted but 'newlines-between' is

B4nan · 2024-11-06T15:10:45Z

index.js

+            // Allow less strict formatting of function parentheses.
+            'function-paren-newline': 'off',
+            // Prefer destructuring for certain cases only. Forcing destructuring for arrays can make the code less readable.
+            'prefer-destructuring': ['error', {


(personal pet peeve)

If I could change one thing in how we write things in Apify, it would be this. We overuse (object) destructing so hard in here 😿

Can you provide some examples, where you feel we overuse destructuring?

Hmm, wondering if the correct word is destructuring or destructing

All the open source repos had this enabled and use it extensively, same for actors. My problem with this is that you loose discoverability, as well as ability to mutate things. Wanna see what is available in actor context? You need to go to the function params instead of just writing ctx.. Wanna change some option we destruct on runtime? Sorry, cant do, since we introduced a new local var for it and used that instead. Also it feels like it results in more code while it was (imho) introduced to make us write less. Lastly, it forces people to do weird things like destructing process.env, which again suffers from the immutability, making tests of env vars much harder.

But maybe its just me 🙃

loose discoverability, as well as ability to mutate things

sorry for stalking but :this: 💯 👍

I generally think that immutability is good, but the loss of discoverability is real. Reading the choose-your-adventure style code is a pain.

Hmm, wondering if the correct word is destructuring or destructing

Oh lord, I have been calling it wrong the whole time, thanks for mentioning :D

B4nan · 2024-11-06T15:15:50Z

index.js

+        },
+
+        rules: {
+            indent: ['error', 4, {


this is quite buggy (and deprecated) rule, it screws up with decorators (which is valid for the console nest app), they had quite a lot of issues about this, closed as wontfix, suggesting people to use formatters instead of it

Yeah, formatting will be the next step, but it would make scope of the project too large, so I'll keep it here until we move forward with prettier in the future

yes, my point is we should not use this rule, not that we should add prettier right ahead. its broken and console team will have issues with that in the nest repo most likely.

index.js

package.json

jirimoravcik · 2024-11-07T13:39:26Z

Hello @janbuchar I borrowed the nice release actions you made for Apify client. Can you check that they're setup alright here? I've done 2 changes: Removed wait for checks step and removed build step (before publish). Thanks

FYI the 2 secrets used are inherited from org so they should be available.

Also one question, will it behave ok since we don't have package-lock.json in this repo? and also no CHANGELOG.md?

janbuchar

@jirimoravcik yeah, I believe it will work fine.

janbuchar · 2024-11-07T14:38:00Z

.github/workflows/pre_release.yaml

+            -   name: Update CHANGELOG.md
+                uses: DamianReeves/write-file-action@master
+                with:
+                    path: CHANGELOG.md
+                    write-mode: overwrite
+                    contents: ${{ needs.release_metadata.outputs.changelog }}


If you don't want to keep a changelog for some reason, then you can delete this. Same applies to release.yaml.

.github/workflows/release.yaml

.github/workflows/pre_release.yaml

feat!: flat config support

924d1c5

jirimoravcik added the adhoc Ad-hoc unplanned task added during the sprint. label Oct 11, 2024

github-actions bot assigned jirimoravcik Oct 11, 2024

github-actions bot added this to the 100th sprint - Platform Team milestone Oct 11, 2024

github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Oct 11, 2024

jirimoravcik added 11 commits October 11, 2024 15:50

remove unused dependencies

d622f94

globals

65e4ab4

consistency

dfb31b4

override eslint in airbnb config

5335112

improve ts configuration, add comments

7dc3cf8

fix

bae1ea7

more cleanup

06de63c

continue with the changes

d3351f0

update comment

e90b806

readme, more comments

8e47507

import no default export

65ff5ce

jirimoravcik marked this pull request as ready for review October 31, 2024 14:15

jirimoravcik removed the adhoc Ad-hoc unplanned task added during the sprint. label Oct 31, 2024

jirimoravcik changed the title ~~feat!: flat config support~~ feat!: flat config + v9 support, rule unification, add TS config Oct 31, 2024

package bump

ac287a1

jirimoravcik requested review from B4nan, fnesveda, gippy, metalwarrior665, stetizu1 and Jkuzz October 31, 2024 14:52

metalwarrior665 reviewed Oct 31, 2024

View reviewed changes

fnesveda requested changes Nov 1, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

jirimoravcik and others added 2 commits November 1, 2024 14:31

Update index.js

bbcd203

Co-authored-by: František Nesveda <[email protected]>

Update index.js

bcb97f5

Co-authored-by: František Nesveda <[email protected]>

stetizu1 approved these changes Nov 6, 2024

View reviewed changes

gippy approved these changes Nov 6, 2024

View reviewed changes

Strajk approved these changes Nov 6, 2024

View reviewed changes

some more changes

a8cabec

jirimoravcik requested a review from metalwarrior665 November 6, 2024 11:52

B4nan reviewed Nov 6, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

jirimoravcik added 4 commits November 7, 2024 10:56

optional peer dep

43d28e8

readme newline

20dcfd5

peer deps versions

17443d2

final touches

d0be0bb

jirimoravcik requested a review from fnesveda November 7, 2024 10:55

jirimoravcik added 3 commits November 7, 2024 14:03

borrow release logic from apify client

59f24be

Merge branch 'master' into feat/flat-config-support

bbde0f5

cleanup old action

e0b34f1

jirimoravcik requested a review from janbuchar November 7, 2024 13:38

janbuchar reviewed Nov 7, 2024

View reviewed changes

PR review

3663ea0

jirimoravcik merged commit e09b4e7 into master Nov 7, 2024
1 check passed

jirimoravcik deleted the feat/flat-config-support branch November 7, 2024 15:24

fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: flat config + v9 support, rule unification, add TS config #14

feat!: flat config + v9 support, rule unification, add TS config #14

jirimoravcik commented Oct 11, 2024 •

edited

Loading

metalwarrior665 Oct 31, 2024

Jkuzz Nov 1, 2024

stetizu1 Nov 6, 2024

B4nan Nov 6, 2024

fnesveda left a comment

stetizu1 Nov 6, 2024

B4nan left a comment

B4nan Nov 6, 2024

jirimoravcik Nov 7, 2024

B4nan Nov 7, 2024

jirimoravcik Nov 7, 2024

B4nan Nov 7, 2024

B4nan Nov 6, 2024 •

edited

Loading

jirimoravcik Nov 7, 2024

B4nan Nov 7, 2024

B4nan Nov 6, 2024

jirimoravcik Nov 7, 2024

B4nan Nov 7, 2024

Strajk Nov 7, 2024 •

edited

Loading

janbuchar Nov 7, 2024

B4nan Nov 7, 2024

B4nan Nov 6, 2024

jirimoravcik Nov 7, 2024

B4nan Nov 7, 2024

jirimoravcik commented Nov 7, 2024 •

edited

Loading

janbuchar left a comment

janbuchar Nov 7, 2024

feat!: flat config + v9 support, rule unification, add TS config #14

feat!: flat config + v9 support, rule unification, add TS config #14

Conversation

jirimoravcik commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnesveda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B4nan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B4nan Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strajk Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirimoravcik commented Nov 7, 2024 • edited Loading

janbuchar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jirimoravcik commented Oct 11, 2024 •

edited

Loading

B4nan Nov 6, 2024 •

edited

Loading

Strajk Nov 7, 2024 •

edited

Loading

jirimoravcik commented Nov 7, 2024 •

edited

Loading