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

chore(datastore): remove tslint and enable eslint #13316

Merged
merged 7 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 0 additions & 20 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,26 +54,6 @@ module.exports = {
'packages/rtn-push-notification/__tests__',
'packages/rtn-web-browser/__tests__',
// 'packages/storage/__tests__',
// will enable lint by packages
// 'adapter-nextjs',
// 'packages/analytics',
// 'packages/api',
// 'packages/api-graphql',
// 'packages/api-rest',
// 'packages/auth',
// 'packages/aws-amplify',
// 'packages/core',
'packages/datastore',
'packages/datastore-storage-adapter',
// 'packages/geo',
// 'packages/interactions',
// 'packages/notifications',
// 'packages/predictions',
// 'packages/pubsub',
// 'packages/react-native',
// 'packages/rtn-push-notification',
// 'packages/rtn-web-browser',
// 'packages/storage',
],
rules: {
camelcase: [
Expand Down
3 changes: 2 additions & 1 deletion packages/datastore-storage-adapter/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
"build": "npm run clean && npm run build:esm-cjs && npm run build:umd",
"clean": "rimraf dist lib lib-esm",
"format": "echo \"Not implemented\"",
"lint": "tslint '{__tests__,src}/**/*.ts' && npm run ts-coverage",
"lint": "eslint '**/*.{ts,tsx}' && npm run ts-coverage",
"lint:fix": "eslint '**/*.{ts,tsx}' --fix",
"ts-coverage": "typescript-coverage-report -p ./tsconfig.build.json -t 94.16"
},
"repository": {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import { CommonSQLiteAdapter } from '../common/CommonSQLiteAdapter';

import ExpoSQLiteDatabase from './ExpoSQLiteDatabase';

const ExpoSQLiteAdapter: CommonSQLiteAdapter = new CommonSQLiteAdapter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@
import { ConsoleLogger } from '@aws-amplify/core';
import { PersistentModel } from '@aws-amplify/datastore';
import { deleteAsync, documentDirectory } from 'expo-file-system';
import { openDatabase, WebSQLDatabase } from 'expo-sqlite';
import { WebSQLDatabase, openDatabase } from 'expo-sqlite';

import { DB_NAME } from '../common/constants';
import { CommonSQLiteDatabase, ParameterizedStatement } from '../common/types';

const logger = new ConsoleLogger('ExpoSQLiteDatabase');

/*

Note:
ExpoSQLite transaction error callbacks require returning a boolean value to indicate whether the
error was handled or not. Returning a true value indicates the error was handled and does not
Note:
ExpoSQLite transaction error callbacks require returning a boolean value to indicate whether the
error was handled or not. Returning a true value indicates the error was handled and does not
rollback the whole transaction.

*/
Expand Down Expand Up @@ -56,6 +57,7 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
params: (string | number)[],
): Promise<T> {
const results: T[] = await this.getAll(statement, params);

return results[0];
}

Expand All @@ -74,6 +76,7 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
(_, error) => {
reject(error);
logger.warn(error);

return true;
},
);
Expand All @@ -93,6 +96,7 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
(_, error) => {
reject(error);
logger.warn(error);

return true;
},
);
Expand All @@ -101,24 +105,27 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
}

public batchQuery<T = any>(
queryParameterizedStatements: Set<ParameterizedStatement> = new Set(),
queryParameterizedStatements = new Set<ParameterizedStatement>(),
): Promise<T[]> {
return new Promise((resolveTransaction, rejectTransaction) => {
return new Promise((resolve, reject) => {
const resolveTransaction = resolve;
const rejectTransaction = reject;
this.db.transaction(async transaction => {
try {
const results: any[] = await Promise.all(
[...queryParameterizedStatements].map(
([statement, params]) =>
new Promise((resolve, reject) => {
new Promise((_resolve, _reject) => {
transaction.executeSql(
statement,
params,
(_, result) => {
resolve(result.rows._array[0]);
_resolve(result.rows._array[0]);
},
(_, error) => {
reject(error);
_reject(error);
logger.warn(error);

return true;
},
);
Expand All @@ -135,26 +142,29 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
}

public batchSave(
saveParameterizedStatements: Set<ParameterizedStatement> = new Set(),
saveParameterizedStatements = new Set<ParameterizedStatement>(),
deleteParameterizedStatements?: Set<ParameterizedStatement>,
): Promise<void> {
return new Promise((resolveTransaction, rejectTransaction) => {
return new Promise((resolve, reject) => {
const resolveTransaction = resolve;
const rejectTransaction = reject;
this.db.transaction(async transaction => {
try {
// await for all sql statements promises to resolve
await Promise.all(
[...saveParameterizedStatements].map(
([statement, params]) =>
new Promise((resolve, reject) => {
new Promise((_resolve, _reject) => {
transaction.executeSql(
statement,
params,
() => {
resolve(null);
_resolve(null);
},
(_, error) => {
reject(error);
_reject(error);
logger.warn(error);

return true;
},
);
Expand All @@ -165,20 +175,21 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
await Promise.all(
[...deleteParameterizedStatements].map(
([statement, params]) =>
new Promise((resolve, reject) =>
new Promise((_resolve, _reject) => {
transaction.executeSql(
statement,
params,
() => {
resolve(null);
_resolve(null);
},
(_, error) => {
reject(error);
_reject(error);
logger.warn(error);

return true;
},
),
),
);
}),
),
);
}
Expand All @@ -198,33 +209,37 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
const [queryStatement, queryParams] = queryParameterizedStatement;
const [deleteStatement, deleteParams] = deleteParameterizedStatement;

return new Promise((resolveTransaction, rejectTransaction) => {
return new Promise((resolve, reject) => {
const resolveTransaction = resolve;
const rejectTransaction = reject;
this.db.transaction(async transaction => {
try {
const result: T[] = await new Promise((resolve, reject) => {
const result: T[] = await new Promise((_resolve, _reject) => {
Comment on lines -204 to +217
Copy link
Member

Choose a reason for hiding this comment

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

Not thrilled with this. With _ prefixed names indicating an unused variable, this just reads wrong. This feels like a consequence of the linter rule I complained about a little previously which prevents resolve, reject from being named anything else.

Does it make sense to drop that rule?

Copy link
Member

@svidgen svidgen May 1, 2024

Choose a reason for hiding this comment

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

I see lots of other places the _ was also used for disambiguation between contexts. Not super thrilling. And, I'd still be interested in turning off the promise arg naming rule, personally. But, I don't want to block on this at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can discuss this and adjust the settings. Though this particular change was for the variable shadowing rule - that we had nested promise executors implementation.

transaction.executeSql(
queryStatement,
queryParams,
(_, result) => {
resolve(result.rows._array || []);
(_, sqlResult) => {
_resolve(sqlResult.rows._array || []);
},
(_, error) => {
reject(error);
_reject(error);
logger.warn(error);

return true;
},
);
});
await new Promise((resolve, reject) => {
await new Promise((_resolve, _reject) => {
transaction.executeSql(
deleteStatement,
deleteParams,
() => {
resolve(null);
_resolve(null);
},
(_, error) => {
reject(error);
_reject(error);
logger.warn(error);

return true;
},
);
Expand All @@ -239,21 +254,24 @@ class ExpoSQLiteDatabase implements CommonSQLiteDatabase {
}

private executeStatements(statements: string[]): Promise<void> {
return new Promise((resolveTransaction, rejectTransaction) => {
return new Promise((resolve, reject) => {
const resolveTransaction = resolve;
const rejectTransaction = reject;
this.db.transaction(async transaction => {
try {
await Promise.all(
statements.map(
statement =>
new Promise((resolve, reject) => {
new Promise((_resolve, _reject) => {
transaction.executeSql(
statement,
[],
() => {
resolve(null);
_resolve(null);
},
(_, error) => {
reject(error);
_reject(error);

return true;
},
);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
import { CommonSQLiteAdapter } from '../common/CommonSQLiteAdapter';

import SQLiteDatabase from './SQLiteDatabase';

const SQLiteAdapter: CommonSQLiteAdapter = new CommonSQLiteAdapter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import SQLite from 'react-native-sqlite-storage';
import { ConsoleLogger } from '@aws-amplify/core';
import { PersistentModel } from '@aws-amplify/datastore';

import { DB_NAME } from '../common/constants';
import { CommonSQLiteDatabase, ParameterizedStatement } from '../common/types';

Expand All @@ -16,7 +17,7 @@ if (ConsoleLogger.LOG_LEVEL === 'DEBUG') {

/*

Note:
Note:
I purposely avoided using arrow functions () => {} in this class,
Because I ran into issues with them in some of the SQLite method callbacks

Expand All @@ -41,7 +42,7 @@ class SQLiteDatabase implements CommonSQLiteDatabase {
}

public async createSchema(statements: string[]): Promise<void> {
return await this.executeStatements(statements);
await this.executeStatements(statements);
}

public async clear(): Promise<void> {
Expand All @@ -56,6 +57,7 @@ class SQLiteDatabase implements CommonSQLiteDatabase {
params: (string | number)[],
): Promise<T> {
const results: T[] = await this.getAll(statement, params);

return results[0];
}

Expand Down Expand Up @@ -138,7 +140,14 @@ class SQLiteDatabase implements CommonSQLiteDatabase {
},
logger.warn,
);
tx.executeSql(deleteStatement, deleteParams, () => {}, logger.warn);
tx.executeSql(
deleteStatement,
deleteParams,
() => {
// no-op
},
logger.warn,
);
});

return results;
Expand Down
Loading
Loading