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

Remove deprecated 3.x APIs in core-common #7572

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

MichaelSwigerAtBentley
Copy link
Contributor

@MichaelSwigerAtBentley MichaelSwigerAtBentley commented Jan 17, 2025

Removes deprecated APIs from core-common package

Note: This PR does not remove RPC methods deprecated in 3.6 or update the RPC interface. Deferring those changes for a larger scale RPC PR.

Files not edited in favor of a later RPC specific PR:
BackendTypes.ts
RpcConstants.ts
OpenAPI.ts
WebAppRpcProtocol.ts

This reverts commit 76974ed.

Revert BackgroundMapSettings Removal
@MichaelSwigerAtBentley
Copy link
Contributor Author

There are also deprecated objects in rpc, such as HttpServerRequest and HttpServerResponse with the note, "The RPC system will be significantly refactored (or replaced) in the future.". These objects are used throughout the rest of the files. Are these objects we wish to no longer export, or make private?

@pmconne
Copy link
Member

pmconne commented Jan 17, 2025

deprecated objects in rpc

My strong opinion is to leave RPC to a separate PR. It seems unlikely that most of the deprecated stuff will actually be removable in the 5.0 timeline.

@MichaelSwigerAtBentley
Copy link
Contributor Author

deprecated objects in rpc

My strong opinion is to leave RPC to a separate PR. It seems unlikely that most of the deprecated stuff will actually be removable in the 5.0 timeline.

Alright, I'll leave it out for now and create a new issue to track it separately.

}

/** @deprecated in 3.x. Use [CreateRenderMaterialArgs]($frontend). */
export class Params {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These params are also still in use, I'm going to try swapping them out for CreateRenderMaterialArgs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RenderTexture.Params is being using in multiple places in core\frontend

I was able to change one of the files, ParseImdlDoucment.ts, but I'm not sure if its implemented correctly as there is some logic around parsing colors.

@aruniverse @ben-polinsky Who would be best to talk to for more info on how to proceed.

Copy link
Member

Choose a reason for hiding this comment

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

@pmconne will be your best starting point

Copy link
Member

Choose a reason for hiding this comment

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

Looks like some of the core-frontend usage is being cleaned up in #7591.

Bear in mind that removing deprecated APIs just means making them unavailable to consumers of the public API. While generally we'll want to delete the APIs entirely, if it's too complicated to clean up our internal usage of a particular one, we can instead move it to /src/internal, and export it from cross-package.ts if it's used in other packages within itwinjs-core.

@MichaelSwigerAtBentley
Copy link
Contributor Author

Separate Issue for RPC: https://github.com/iTwin/itwinjs-backlog/issues/1374

@MichaelSwigerAtBentley MichaelSwigerAtBentley marked this pull request as ready for review January 31, 2025 17:53
Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

here's a guide you can use as reference for the internal apis:
https://www.itwinjs.org/learning/guidelines/release-tags-guidelines/#internal-apis

Copy link
Member

Choose a reason for hiding this comment

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

we should move this file into core/common/src/internal

/** @deprecated in 3.x. Use [CreateRenderMaterialArgs]($frontend). */
/** Params for use in old CreateMaterial functions. Use [CreateRenderMaterialArgs]($frontend) instead.
* @internal
*/
export class Params {
Copy link
Member

Choose a reason for hiding this comment

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

grep for RenderMaterial. in the repo. you'll see RenderMaterial.Params is still used in few places

the namespace here looks useless now that the only thing exposed is marked internal, wonder if we deprecate the namespace or just mark it as internal too, or honestly remove it entirely and move the Params class into core/common/src/internal

* @deprecated in 3.x. use RenderSystem.createTexture and CreateTextureArgs.
* @public
* Use RenderSystem.createTexture and CreateTextureArgs.
* @internal
Copy link
Member

Choose a reason for hiding this comment

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

same comments here as in RenderMaterial.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants