Deprecate UseJsPropertyNames and ECSqlStatement (which defaults to UseJsPropertyNames) #7315

ColinKerr · 2024-11-01T18:56:27Z

No description provided.

core/backend/src/ECSqlStatement.ts

aruniverse · 2024-11-05T14:46:27Z

@Mergifyio backport release/4.10.x

mergify · 2024-11-05T14:46:33Z

backport release/4.10.x

🟠 Waiting for conditions to match

merged [📌 backport requirement]

aruniverse

lets include a comment about this in NextVersion.md.
That can be done in the bp pr to release/4.10.x branch

…eprecate-ecsql-stuff

aruniverse · 2025-01-29T21:43:02Z

https://github.com/Mergifyio backport release/4.11.x

mergify · 2025-01-29T21:43:13Z

backport release/4.11.x

🟠 Waiting for conditions to match

merged [📌 backport requirement]

aruniverse · 2025-01-29T21:43:47Z

lets include a comment about this in NextVersion.md. That can be done in the bp pr to release/4.10.x branch

doc changes can go into release/4.11.x branch

…eprecate-ecsql-stuff

ColinKerr · 2025-02-04T19:25:21Z

I cannot approve because I created the PR, but it's looking good to me.

The only thing I can see missing is a note in next version md covering changes

aruniverse

does it make sense for the example code snippets that get pulled into the docs site use the new api instead of suppressing the deprecation warnings?

core/common/src/ConcurrentQuery.ts

core/backend/src/IModelDb.ts

core/backend/src/ECSqlStatement.ts

core/backend/src/ECDb.ts

Co-authored-by: Arun George <[email protected]>

Update deprecated in version to 4.11 Co-authored-by: Arun George <[email protected]>

aruniverse · 2025-02-05T16:47:23Z

I cannot approve because I created the PR, but it's looking good to me.

The only thing I can see missing is a note in next version md covering changes

@rschili would you be able to review since @ColinKerr and @khanaffan both worked on the pr?
Although @khanaffan should be able to provide an approval since he didnt create the pr

pmconne · 2025-02-05T17:20:54Z

core/backend/src/ChangeSummaryManager.ts

-        stmt.bindString(1, changeSetId);
-        if (DbResult.BE_SQLITE_ROW === stmt.step())
-          return stmt.getValue(0).getId();
+    // eslint-disable-next-line @typescript-eslint/no-deprecated


Why ignore deprecation warnings instead of updating to use preferred replacement APIs? (Here and lots of other places).

I have the same concerns, but the blast radius here is quite a lot, so I'm ok if we are temporarily suppressing the deprecation warnings internally. But I would hope and require we use the new api in our documentation with this pr, and have examples (in example-code snippets)

As I understand, we do not offer a sync replacement for the deprecated API, so to switch to ConcurrentQuery in a clean way we may have to make the caller methods async as well. Which would increase the blast radius even more.

Is there some reason to believe the people consuming the deprecated APIs will not experience similar friction in attempting to address the new deprecation warnings in their own code? It sounds like our replacement APIs are insufficient, which suggests deprecation may be premature.

khanaffan · 2025-02-05T17:33:01Z

I cannot approve because I created the PR, but it's looking good to me.
The only thing I can see missing is a note in next version md covering changes

@rschili would you be able to review since @ColinKerr and @khanaffan both worked on the pr? Although @khanaffan should be able to provide an approval since he didnt create the pr

I have updated the nextversion.md

aruniverse · 2025-02-05T17:36:09Z

I cannot approve because I created the PR, but it's looking good to me.
The only thing I can see missing is a note in next version md covering changes

@rschili would you be able to review since @ColinKerr and @khanaffan both worked on the pr? Although @khanaffan should be able to provide an approval since he didnt create the pr

I have updated the nextversion.md

this should go in the bp to 4.11 not on master

khanaffan · 2025-02-05T18:47:08Z

I cannot approve because I created the PR, but it's looking good to me.
The only thing I can see missing is a note in next version md covering changes

@rschili would you be able to review since @ColinKerr and @khanaffan both worked on the pr? Although @khanaffan should be able to provide an approval since he didnt create the pr

I have updated the nextversion.md

this should go in the bp to 4.11 not on master

All changes should be committed to the master branch first, as it serves as our source of truth. Deprecating a feature in version 4.x implies it is also deprecated in version 5.x. Currently, we do not have a timeline for updating the code in iTwin.js 5.x to transition from ECSQL statements to ECSQLReader."

aruniverse · 2025-02-05T20:40:03Z

I cannot approve because I created the PR, but it's looking good to me.
The only thing I can see missing is a note in next version md covering changes

@rschili would you be able to review since @ColinKerr and @khanaffan both worked on the pr? Although @khanaffan should be able to provide an approval since he didnt create the pr

I have updated the nextversion.md

this should go in the bp to 4.11 not on master

All changes should be committed to the master branch first, as it serves as our source of truth. Deprecating a feature in version 4.x implies it is also deprecated in version 5.x.

Agreed, however the NextVersion.md file is a special file we use to generate the master changelog for a a major or minor release. The way now, the deprecation changes will be in the 5.0.0.md file where it should instead be in the 4.11.0.md files. See previous version change logs here https://github.com/iTwin/itwinjs-core/tree/master/docs/changehistory

rschili · 2025-02-05T20:52:09Z

I cannot approve because I created the PR, but it's looking good to me.
The only thing I can see missing is a note in next version md covering changes

@rschili would you be able to review

Will do first thing tomorrow.

grigasp · 2025-02-06T07:49:56Z

I updated Presentation code to use recommended APIs instead of deprecated ones. @saskliutas please review.

rschili

Please review my concerns about caller methods who continue to use Statement. Is there a clean sync replacement for statement or do we have to make all callers async once we switch to ConcurrentQuery? If so, we should deprecate the remaining callers as well and provide async replacements...

rschili · 2025-02-06T09:43:30Z

core/backend/src/IModelDb.ts

@@ -1229,6 +1243,7 @@ export abstract class IModelDb extends IModel {
   */
  public querySchemaVersion(schemaName: string): string | undefined {
    const sql = `SELECT VersionMajor,VersionWrite,VersionMinor FROM ECDbMeta.ECSchemaDef WHERE Name=:schemaName LIMIT 1`;
+    // eslint-disable-next-line @typescript-eslint/no-deprecated


I believe we need to deprecate this method as well. We cannot rewrite it to async without breaking its signature. Once we remove statement this will have to become async

rschili · 2025-02-06T09:44:03Z

core/backend/src/IModelDb.ts

@@ -1613,6 +1628,7 @@ export namespace IModelDb {
     */
    public queryLastModifiedTime(modelId: Id64String): string {
      const sql = `SELECT LastMod FROM ${Model.classFullName} WHERE ECInstanceId=:modelId`;
+      // eslint-disable-next-line @typescript-eslint/no-deprecated


This method probably needs to be turned async/deprecated as well

There are more occurrences along the rest of the file

rschili · 2025-02-06T09:52:23Z

core/backend/src/ChangeSummaryManager.ts

-        stmt.bindString(1, changeSetId);
-        if (DbResult.BE_SQLITE_ROW === stmt.step())
-          return stmt.getValue(0).getId();
+    // eslint-disable-next-line @typescript-eslint/no-deprecated


As I understand, we do not offer a sync replacement for the deprecated API, so to switch to ConcurrentQuery in a clean way we may have to make the caller methods async as well. Which would increase the blast radius even more.

First cut deprecation

9ff3334

khanaffan reviewed Nov 4, 2024

View reviewed changes

core/backend/src/ECSqlStatement.ts Outdated Show resolved Hide resolved

aruniverse reviewed Nov 6, 2024

View reviewed changes

Merge branch 'master' of https://github.com/iTwin/itwinjs-core into d…

da8a887

…eprecate-ecsql-stuff

khanaffan added 7 commits January 30, 2025 13:53

supress linter errors

b2e120e

extract-api

65b7403

Merge branch 'master' of https://github.com/iTwin/itwinjs-core into d…

3085036

…eprecate-ecsql-stuff

fix errors

df14532

lint

17072c4

fix test

3aa55cd

Merge branch 'master' of https://github.com/iTwin/itwinjs-core into d…

1138c84

…eprecate-ecsql-stuff

khanaffan marked this pull request as ready for review February 4, 2025 19:15

khanaffan requested review from a team, bbastings, diegoalexdiaz, jchick-bentley, hl662 and rschili as code owners February 4, 2025 19:15

aruniverse reviewed Feb 4, 2025

View reviewed changes

ColinKerr and others added 3 commits February 5, 2025 08:31

Update core/common/src/ConcurrentQuery.ts

785d426

Co-authored-by: Arun George <[email protected]>

Update core/common/src/ConcurrentQuery.ts

cceb45c

Co-authored-by: Arun George <[email protected]>

Apply suggestions from code review

4458a04

Update deprecated in version to 4.11 Co-authored-by: Arun George <[email protected]>

aruniverse and others added 3 commits February 5, 2025 11:08

Apply suggestions from code review

ad46313

Merge branch 'master' into deprecate-ecsql-stuff

cd4a38f

rush changes

31e83a8

pmconne reviewed Feb 5, 2025

View reviewed changes

update next version MD

3cd2d25

khanaffan requested a review from a team as a code owner February 5, 2025 17:32

khanaffan requested a review from paulius-valiunas February 5, 2025 17:32

Merge branch 'master' into deprecate-ecsql-stuff

f15afca

khanaffan and others added 2 commits February 5, 2025 17:27

Merge branch 'master' into deprecate-ecsql-stuff

dce0534

use recommended apis instead of deprecated ones

ebd1548

saskliutas approved these changes Feb 6, 2025

View reviewed changes

rschili reviewed Feb 6, 2025

View reviewed changes

Merge branch 'master' into deprecate-ecsql-stuff

fb30951

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate UseJsPropertyNames and ECSqlStatement (which defaults to UseJsPropertyNames) #7315

Deprecate UseJsPropertyNames and ECSqlStatement (which defaults to UseJsPropertyNames) #7315

ColinKerr commented Nov 1, 2024

aruniverse commented Nov 5, 2024

mergify bot commented Nov 5, 2024

aruniverse left a comment

aruniverse commented Jan 29, 2025

mergify bot commented Jan 29, 2025

aruniverse commented Jan 29, 2025

ColinKerr commented Feb 4, 2025

aruniverse left a comment

aruniverse commented Feb 5, 2025

pmconne Feb 5, 2025

aruniverse Feb 5, 2025

rschili Feb 6, 2025

pmconne Feb 6, 2025 •

edited

Loading

khanaffan commented Feb 5, 2025

aruniverse commented Feb 5, 2025

khanaffan commented Feb 5, 2025 •

edited

Loading

aruniverse commented Feb 5, 2025

rschili commented Feb 5, 2025

grigasp commented Feb 6, 2025

rschili left a comment

rschili Feb 6, 2025

rschili Feb 6, 2025

rschili Feb 6, 2025

rschili Feb 6, 2025

Deprecate UseJsPropertyNames and ECSqlStatement (which defaults to UseJsPropertyNames) #7315

Are you sure you want to change the base?

Deprecate UseJsPropertyNames and ECSqlStatement (which defaults to UseJsPropertyNames) #7315

Conversation

ColinKerr commented Nov 1, 2024

aruniverse commented Nov 5, 2024

mergify bot commented Nov 5, 2024

🟠 Waiting for conditions to match

aruniverse left a comment

Choose a reason for hiding this comment

aruniverse commented Jan 29, 2025

mergify bot commented Jan 29, 2025

🟠 Waiting for conditions to match

aruniverse commented Jan 29, 2025

ColinKerr commented Feb 4, 2025

aruniverse left a comment

Choose a reason for hiding this comment

aruniverse commented Feb 5, 2025

pmconne Feb 5, 2025

Choose a reason for hiding this comment

aruniverse Feb 5, 2025

Choose a reason for hiding this comment

rschili Feb 6, 2025

Choose a reason for hiding this comment

pmconne Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

khanaffan commented Feb 5, 2025

aruniverse commented Feb 5, 2025

khanaffan commented Feb 5, 2025 • edited Loading

aruniverse commented Feb 5, 2025

rschili commented Feb 5, 2025

grigasp commented Feb 6, 2025

rschili left a comment

Choose a reason for hiding this comment

rschili Feb 6, 2025

Choose a reason for hiding this comment

rschili Feb 6, 2025

Choose a reason for hiding this comment

rschili Feb 6, 2025

Choose a reason for hiding this comment

rschili Feb 6, 2025

Choose a reason for hiding this comment

pmconne Feb 6, 2025 •

edited

Loading

khanaffan commented Feb 5, 2025 •

edited

Loading