Deprecate IDisposable and related utilities

[wgoehrig]

Since TypeScript 5.2 added support for the explicit resource management feature in ECMAScript, let's deprecate IDisposable and use the new built-in Disposable instead.

[pmconne]

I want to see what output the linter produces. You've presumably introduced a ton of deprecation warnings.

I'd remove all the implements IDisposables from our code - they all implicitly implement it and you'd otherwise presumably have to suppress the lint rule at each occurrence.

[wgoehrig]

This is actually passing lint without any errors in its current state - looks like the @typescript-eslint/no-deprecated rule just doesn't check implements clauses...

[pmconne]

I'd remove implements IDisposable everywhere anyway, no reason to document implementation of an interface we discourage people from using.

[aruniverse]

Over the last few weeks I have been running into issues in my deployed app where it complains about "Uncaught SyntaxError: Missing initializer in using declaration".

I'm not necessarily sure if this is browser specific or not, I know others are not able to repro with the same version of chrome(Version 131.0.6778.109 (Official Build) (arm64)), and if I switch browsers it works fine.

I havent investigated too much, but I think the changes in this PR would help solve it. Not sure why when I build my app, depending on the browser it decides to use the global using instead of using from core-bentley

[dassaf4]

core-geometry change is good.

[aruniverse]

nit: idk if you should explicitly mention a future version, and i dont think you need to call out the support policy

[aruniverse]

nit: missing in the ToC above
only update this if you are reacting to the other comment on this file. this will get fixed automatically by the next time someone edits this file and has markdownlint/formatters enabled

[tcobbs-bentley]

Mobile changes look fine.

[grigasp]

Presentation: did some cleanup, fixed coverage not being 100%. Thanks for taking this up!

[mergify]

This pull request is now in conflicts. Could you fix it @wgoehrig? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

[grigasp]

I've been making similar changes in presentation repo and came up with a situation that concerns me. I'm not sure it's really something we should worry about, but just wanted to raise this and hear your opinion.

Here's a hypothetical situation before the changes:

class Ours {
  public dispose() {
    // do our dispose
  }
}
class Theirs extends Ours {
  public override dispose() {
    super.dispose();
    // do their dispose
  }
}
function useAndDispose(factory: () => Ours) {
  const instance = factory();
  // ... do something with instance
  // the below call does theirs as well as ours dispose:
  instance.dispose();
}

There could be problems with Theirs.dispose not being called after the changes:

class Ours {
  public [Symbol.dispose]() {
    // do our dispose
  }
  /** @deprecated */
  public dispose() {
    this[Symbol.dispose]();
  }
}
// this class has not been updated to use the new disposal method:
class Theirs extends Ours {
  public override dispose() {
    super.dispose();
    // do their dispose
  }
}
function useAndDispose(factory: () => Ours) {
  // this won't do "theirs dispose"!
  using instance = factory();

  // this wouldn't either:
  const instance = factory();    
  instance[Symbol.dispose]();

  // and even this wouldn't:
  const instance = factory();    
  if (isDisposable(instance)) {
    instance[Symbol.dispose]();
  } else if (isIDisposable(instance)) {
    instance.dispose();
  }

  // only this would:
  const instance = factory();    
  if (isIDisposable(instance)) {
    instance.dispose();
  } else if (isDisposable(instance)) {
    instance[Symbol.dispose]();
  }
}

It seems we should be very careful about only calling Symbol.dispose if the instance we're calling it on is not 100% created by ourselves.

[wgoehrig]

It seems we should be very careful about only calling Symbol.dispose if the instance we're calling it on is not 100% created by ourselves.

I agree this is an important consideration - I checked earlier with @pmconne and he seemed confident that we don't have any patterns in at least the render system where ownership is "transferred" in this way (i.e., where we're responsible for the disposal of something passed in). @grigasp are you aware of anywhere that's happening in presentation?

[grigasp]

It seems we should be very careful about only calling Symbol.dispose if the instance we're calling it on is not 100% created by ourselves.

I agree this is an important consideration - I checked earlier with @pmconne and he seemed confident that we don't have any patterns in at least the render system where ownership is "transferred" in this way (i.e., where we're responsible for the disposal of something passed in). @grigasp are you aware of anywhere that's happening in presentation?

There is one place... In the presentation repo I have a safeDispose function which is similar to your dispose, except that it prefers the deprecated dispose over the new Symbol.dispose. What do you think about switching the order in the core-bentley's dispose?

[aruniverse]

Over the last few weeks I have been running into issues in my deployed app where it complains about "Uncaught SyntaxError: Missing initializer in using declaration".

I'm not necessarily sure if this is browser specific or not, I know others are not able to repro with the same version of chrome(Version 131.0.6778.109 (Official Build) (arm64)), and if I switch browsers it works fine.

I havent investigated too much, but I think the changes in this PR would help solve it. Not sure why when I build my app, depending on the browser it decides to use the global using instead of using from core-bentley

So I have unknowingly fixed this in my app. Changed sourcemap config option in vite from inline to true and this fixed the issue for me. So when you inline source maps you have both the using directive and the using utlitity function in the same file. Strange part I dont still dont understand is why this worked for some and not others, and worked in some browsers vs not.

Either way, I think this PR is still in our best interest to complete.

[mergify]

This pull request is now in conflicts. Could you fix it @wgoehrig? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/