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

Design Meeting Notes, 6/21/2023 #54735

Closed
DanielRosenwasser opened this issue Jun 21, 2023 · 5 comments
Closed

Design Meeting Notes, 6/21/2023 #54735

DanielRosenwasser opened this issue Jun 21, 2023 · 5 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 21, 2023

Restricting Mixing module and moduleResolution when one is node*

#54567

  • Stuff gets weird when you mix module and moduleResolution.
  • The first reason to explore this change was we were writing docs that explain what these do.
    • moduleResolution is somewhat straightforward; not module.
    • Conceptually, moduleResolution is strictly path lookups, module decides emit, and is driven by a local package.json, etc.
      • However we've seen that some of this behavior has leaked.
  • So we would like to make the docs clearer, but we need to understand this all better ourselves!
  • What happens when you error when they don't match up? Some examples:
    • cheerio
      • dual CJS/ESM build
      • ESM build does --moduleResolution node16 --module esnext
      • CJS does --moduleResolution node16 --module esnext
        • Seems fine, but is a footgun. Assumes everything will be CJS, resolution will go badly.
      • Node 16 users are getting the wrong types from this package.
      • But the use-case of dual emit is valid and the paths forward today are not great.
    • docusaurus
      • actually should be using --moduleResolution bundler
  • --module esnext --moduleResolution node16?
    • Forcing a dev to say "no the module has to be node16 as well has no downsides.
  • We tried to generalize...but people don't exactly appreciate the degrees of flexibility.
    • Can we deprecate?
  • Aside - so many of the base @tsconfig packages use an invalid mix!
    • --module node16 --moduleResolution node!!!!!
  • Would be great if package.json supported multiple values for the types field depending on path/directory. - but by the time Node runtimes broadly support this, it will be a bit.
  • Feels plausible (desirable) to ban sooner than a long deprecation period.
  • Need to update all the @tsconfig

Naming of Diposable

#54505 (comment)

  • Disposable vs. DisposableLike.
  • It didn't feel ideal to use Iterator for iterator methods, and felt like there was some agreement in committee about placement being off; but ultimately JS can't really support extension methods, and needed an instance and constructor with a prototype so that these Iterator objects can have methods called on them.
  • Disposable feels... different? It's a one-and-done concept. There's no chaining.
  • There also already is an instance one can use today - DisposableStack.
  • So there's less likelihood of it becoming a value global.
  • No other language with a disposable concept (destructors, Drop, IDisposable, ...) has more than a single method.
  • Conclusion: stop worrying and love the destructor.
@fatcerberus
Copy link

CJS does --moduleResolution node16 --module esnext

Surely this was supposed to be --module commonjs

@fatcerberus
Copy link

fatcerberus commented Jun 22, 2023

No other language with a disposable concept (destructors, Drop, IDisposable, ...) has more than a single method.

If we’re classifying destructors as a kind of disposal method, then technically C# has either two or three depending on how you count 😉 (destructor + IDisposable.Dispose, the latter of which has two overloads)--and the way they interact is not super intuitive either FWIR.

@rbuckton
Copy link
Member

No other language with a disposable concept (destructors, Drop, IDisposable, ...) has more than a single method.

If we’re classifying destructors as a kind of disposal method, then technically C# has either two or three depending on how you count 😉 (destructor + IDisposable.Dispose, the latter of which has two overloads)--and the way they interact is not super intuitive either FWIR.

C# has Dispose() for explicit resource management, and finalizers for implicit resource management (e.g., garbage collection). JavaScript will have Symbol.dispose for explicit resource management, and FinalizationRegistry for implicit resource management. A combination of explicit and implicit management is useful for unmanaged handles/file descriptors to ensure they are still closed if the handle wrapper is GC'd but not disposed, and you can do the same thing in both languages:

// C#
class MySafeNativeHandle : IDisposable
{
  private IntPtr unsafeHandle;

  public SafeNativeHandle(IntPtr unsafeHandle)
  {
    this.unsafeHandle = unsafeHandle;
  }

  // implicit resource management
  ~SafeNativeHandle()
  {
    this.DisposeCommon();
  }

  // explicit resource management
  public Dispose()
  {
    GC.SuppressFinalize(this);
    this.DisposeCommon();
  }

  private DisposeCommon()
  {
    if (this.unsafeHandle !== IntPtr.Zero)
    {
      ReleaseHandle(this.unsafeHandle);
      this.unsafeHandle = IntPtr.Zero;
    }
  }
}
// ts
class MySafeNativeHandle implements Disposable {
  #unregisterToken = {};
  #holder: { handle: number; };

  constructor(unsafeNativeHandle: number) {
    this.#holder = { value: unsafeHandle; };

    // register for finalization
    MySafeNativeHandle.#finalizer.register(this, this.#holder, this.#unregisterToken);
  }

  // implicit resource management
  static #finalizer = new FinalizationRegistry<{ handle: number }>(holder => {
    MySafeNativeHandle.#disposeCommon(holder);
  });

  // explicit resource management
  [Symbol.dispose]() {
    MySafeNativeHandle.#finalizer.unregister(this.#unregisterToken);
    MySafeNativeHandle.#disposeCommon(this.#holder);
  }

  static #disposeCommon(holder: { handle: number }) {
    if (holder.handle !== 0) {
      releaseHandle(holder.handle);
      holder.handle = 0;
    }
  }
}

@fatcerberus
Copy link

fatcerberus commented Jun 22, 2023

I seem to recall that C# has a Dispose() overload that takes a disposing boolean parameter (which is really confusing, if I'm calling Dispose then obviously I'm disposing it!)

@rbuckton
Copy link
Member

rbuckton commented Jun 22, 2023

That is for cases when you want to support subclassing. The Dispose(bool disposing) method is essentially the DisposeCommon() method above:

// C#
class MySubclassableSafeNativeHandle : IDisposable
{
  private IntPtr unsafeHandle;

  public MySubclassableSafeNativeHandle(IntPtr unsafeHandle)
  {
    this.unsafeHandle = unsafeHandle;
  }

  // implicit resource management
  ~MySubclassableSafeNativeHandle ()
  {
    this.Dispose(false);
  }

  // explicit resource management
  public Dispose()
  {
    GC.SuppressFinalize(this);
    this.Dispose(true);
  }

  protected virtual Dispose(bool disposing)
  {
    if (this.unsafeHandle !== IntPtr.Zero)
    {
      ReleaseHandle(this.unsafeHandle);
      this.unsafeHandle = IntPtr.Zero;
    }
  }
}

Subclasses can override Dispose(bool disposing) and use the disposing parameter to avoid resurrecting other held object references when it is invoked from a finalizer and not from Dispose(). JavaScript doesn't need this because a FinalizationRegistry can't cause resurrection because the callback is only triggered after an object has already been GC'd and is completely unreachable.

interface FinalizationRegistry<T> {
  register(target: object, heldValue: T, unregisterToken?: object): void;
}

The target argument is monitored for GC, but is not held (as that would prevent GC). The heldValue argument is held (preventing GC) so that it can be passed to the finalizer callback, but is itself not monitored. Passing this as the "held value" would prevent GC so the finalizer callback would never execute. Similarly, passing a closure that holds onto this as the "held value" would also prevent GC.

Conaclos added a commit to bare-ts/bare that referenced this issue Jun 28, 2023
- Don't specify TS `moduleResolution`

  In the next release, it will be an error to set module or
  moduleResolution to node16 and the other to ad istinct value.
  See microsoft/TypeScript#54735

  `module=Node16` implies `moduleResolution=node16`

- Do not preserve quoted properties (prettier, rome)

  We no longer need to preserve quoted properties in the codebase.
  The last object-map was converted to a Map.
Conaclos added a commit to bare-ts/lib that referenced this issue Jul 4, 2023
- Don't specify TS `moduleResolution`

  In the next release, it will be an error to set module or
  moduleResolution to node16 and the other to ad istinct value.
  See microsoft/TypeScript#54735

  `module=Node16` implies `moduleResolution=node16`

- Do not preserve quoted properties (prettier, rome)

  We no longer need to preserve quoted properties in the codebase.
  The last object-map was converted to a Map.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

4 participants