chore(repo): setting up the base for migrating to eslint #12923

HuiSF · 2024-01-29T20:42:47Z

Description of changes

See each the commit message of each commit.

Issue #, if available

Description of how you validated changes

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2024-01-29T20:43:42Z

⚠️ This PR includes manual changes to the "aws-amplify" package.json file, which can have library-wide implications.

Please ensure that this PR:

Does not modify "@aws-amplify/*" dependency versions, which may misalign core dependencies across the library.

A repository administrator is required to review & merge this change.

codecov-commenter · 2024-01-29T21:10:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (38c1d56) 87.97% compared to head (95c7d88) 87.98%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12923      +/-   ##
==========================================
+ Coverage   87.97%   87.98%   +0.01%     
==========================================
  Files         664      664              
  Lines       18187    18187              
  Branches     3647     3647              
==========================================
+ Hits        16000    16002       +2     
+ Misses       2187     2185       -2

Flag	Coverage Δ
adapter-nextjs	`98.64% <ø> (ø)`
analytics	`62.75% <ø> (ø)`
api-graphql	`87.71% <ø> (ø)`
api-rest	`95.95% <ø> (ø)`
auth	`86.78% <ø> (ø)`
aws-amplify	`100.00% <ø> (ø)`
core	`87.72% <ø> (ø)`
datastore	`89.95% <ø> (+0.04%)`	⬆️
datastore-storage-adapter	`88.13% <ø> (ø)`
geo	`90.12% <ø> (ø)`
interactions	`96.10% <ø> (ø)`
notifications	`90.35% <ø> (ø)`
predictions	`86.00% <ø> (ø)`
pubsub	`94.70% <ø> (ø)`
rtn-push-notification	`100.00% <ø> (ø)`
storage	`91.28% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cshfang · 2024-01-29T21:24:54Z

scripts/build.js

@@ -1,251 +0,0 @@
-// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


packages/adapter-nextjs/package.json

ashika112

This looks great Hui 🙏 Thank you

jimblanc

Good stuff! Few questions below

packages/adapter-nextjs/tsconfig.test.json

packages/pubsub/package.json

.eslintrc.js

package.json

calebpollman · 2024-01-30T23:03:18Z

.vscode/settings.json

+	"formattingToggle.affects": [
+		"editor.codeActionsOnSave.source.fixAll.eslint",
+		"editor.formatOnPaste",
+		"editor.formatOnSave",
+		"editor.formatOnType"
+	]


This feels a bit restrictive/opinionated, what is the context of this addition?

This is not restrictive, IMO, but gives the most freedom.

reformat on save is not enabled by default

ppl who are using vscode can install the recommended extension formatting toggle to enable reformat on save (this is the existing set up prior to this migration)

for ppl who are not using auto reformat at all, they will catch formatting issues when run yarn test

Got it, misread these as enforced

calebpollman · 2024-01-30T23:09:50Z

.eslintrc.js

+			},
+		],
+		'valid-typeof': ['error', { requireStringLiterals: false }],
+		'@stylistic/comma-dangle': [


This reads as something that Prettier could just handle?

Prettier handles this (when it actually reformats), this rule is for showing errors with the eslint in details.

Feels extraneous IMO but do not have a strong opinion

calebpollman · 2024-01-30T23:18:04Z

.eslintrc.js

+		'plugin:import/typescript',
+		'plugin:@typescript-eslint/stylistic',
+		'plugin:@typescript-eslint/recommended',
+		"plugin:prettier/recommended",


Would be careful with usage of "plugin:prettier/recommended". Mixing linting with formatting can lead to undesired conflicts

.eslintrc.js

calebpollman · 2024-01-30T23:27:05Z

.eslintrc.js

+		'import/order': ['error', { 'newlines-between': 'always' }],
+		'no-eval': 'error',
+		'no-param-reassign': 'error',
+		'no-shadow': 'off',


Definitely an opinion, but turning off no-shadow can lead to code that is difficult to grok as a reader

Just saw that this is turned off in favor of the @typescript-eslint version 😅

For future maintainers, might be helpful to note which rules are superseded by corresponding @typescript-eslint rules

calebpollman

Left some thoughts/questions mostly from my own battles with configuring eslint.

Definitely an improvement overall, and also plan on using some of these in the UI repo 😏

- move the base tsconfig into the root of the workspace - unifying tsconfig tsconfig.build tsconfig.test and tsconfig.watch settings

- remove unused build.js script

HuiSF requested review from a team as code owners January 29, 2024 20:42

HuiSF marked this pull request as draft January 29, 2024 20:44

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from fd289df to 83db0aa Compare January 29, 2024 21:03

HuiSF marked this pull request as ready for review January 29, 2024 21:15

cshfang reviewed Jan 29, 2024

View reviewed changes

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from 6b7cc95 to 7783301 Compare January 29, 2024 23:13

HuiSF requested a review from cshfang January 29, 2024 23:13

ashika112 previously approved these changes Jan 30, 2024

View reviewed changes

HuiSF dismissed ashika112’s stale review via d2e910c January 30, 2024 00:44

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from 7783301 to d2e910c Compare January 30, 2024 00:44

jimblanc reviewed Jan 30, 2024

View reviewed changes

packages/adapter-nextjs/tsconfig.test.json Outdated Show resolved Hide resolved

packages/pubsub/package.json Outdated Show resolved Hide resolved

.eslintrc.js Show resolved Hide resolved

package.json Show resolved Hide resolved

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from d2e910c to 95c7d88 Compare January 30, 2024 16:56

HuiSF requested a review from jimblanc January 30, 2024 16:57

jimblanc previously approved these changes Jan 30, 2024

View reviewed changes

HuiSF requested a review from ashika112 January 30, 2024 20:38

HuiSF dismissed jimblanc’s stale review via 3486736 January 30, 2024 22:54

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from 95c7d88 to 3486736 Compare January 30, 2024 22:54

calebpollman reviewed Jan 30, 2024

View reviewed changes

jimblanc approved these changes Jan 30, 2024

View reviewed changes

calebpollman reviewed Jan 30, 2024

View reviewed changes

.eslintrc.js Show resolved Hide resolved

calebpollman reviewed Jan 30, 2024

View reviewed changes

calebpollman approved these changes Jan 30, 2024

View reviewed changes

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from 3486736 to 05644e2 Compare January 31, 2024 19:01

HuiSF added 9 commits January 31, 2024 11:30

chore(repo): use typescript 5.0.2 across workspace

e20782e

chore(repo): refactor tsconfig hierarchy

fa0423d

- move the base tsconfig into the root of the workspace - unifying tsconfig tsconfig.build tsconfig.test and tsconfig.watch settings

chore(repo): promote rollup dependencies into workspace

8b3b204

- remove unused build.js script

chore(repo): add build:watch using rollup

dd497d0

chore(repo): setup eslint

872d825

chore(adapter-nextjs): migrate to eslint

5b7a1e9

fix(repo): test-github-actions using js-yaml already removed API

ca85c04

chore(repo): add Code Spell Checker to recommended extension list

c6edaa2

chore(repo): add formatOn actions to formattingToggle controlables

d047026

HuiSF force-pushed the hui/chore/repo/migrate-eslint-new branch from 05644e2 to d047026 Compare January 31, 2024 19:31

iartemiev approved these changes Jan 31, 2024

View reviewed changes

HuiSF merged commit b893b0c into main Jan 31, 2024
30 checks passed

HuiSF deleted the hui/chore/repo/migrate-eslint-new branch January 31, 2024 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(repo): setting up the base for migrating to eslint #12923

chore(repo): setting up the base for migrating to eslint #12923

HuiSF commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024

codecov-commenter commented Jan 29, 2024 •

edited

Loading

cshfang Jan 29, 2024

ashika112 left a comment

jimblanc left a comment

calebpollman Jan 30, 2024

HuiSF Jan 30, 2024

calebpollman Jan 30, 2024

calebpollman Jan 30, 2024

HuiSF Jan 30, 2024

calebpollman Jan 30, 2024

calebpollman Jan 30, 2024

calebpollman Jan 30, 2024

calebpollman Jan 30, 2024

calebpollman left a comment

		@@ -1,251 +0,0 @@
		// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

chore(repo): setting up the base for migrating to eslint #12923

chore(repo): setting up the base for migrating to eslint #12923

Conversation

HuiSF commented Jan 29, 2024 • edited Loading

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

github-actions bot commented Jan 29, 2024

codecov-commenter commented Jan 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ashika112 left a comment

Choose a reason for hiding this comment

jimblanc 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

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

calebpollman left a comment

Choose a reason for hiding this comment

HuiSF commented Jan 29, 2024 •

edited

Loading

codecov-commenter commented Jan 29, 2024 •

edited

Loading