Convert to TS or supply ambient definitions

[chriskrycho]

As part of Ember.js RFC #0800, this package needs to supply type definitions. Preferably, this would happen by converting to TS and generating from the source; as an alternative, it can publish ambient types (presumably starting with the ones on DefinitelyTyped).

All the members of the Typed Ember team are happy to advise on how to do this; feel free to reach out in #dev-typescript on Discord if you have questions!

[chriskrycho]

Note: this will need a public type from Ember for Resolver.

[bwbuchanan]

+@rwjblue

I had a go at converting ember-resolver to TypeScript, basically just adding type annotations and avoiding major code changes. This was tedious but pretty straightforward, and uncovered a few minor bugs in code that failed typechecking.

Some questions that came up:

• Should the addon use ember-source as a devDependency to get access to the types for Resolver, FullName, Factory, KnownForTypeResult?
• What is the public API of ember-resolver? Is it all of the methods and properties that are not underscore-prefixed?
• What would break by refactoring to use ES class syntax instead of EmberObject.extend()? Are there downstream consumers of ember-resolver that subclass it and depend on some particular quirks of EmberObject inheritance versus ES class inheritance?
• Does the resolver need to stay an instance of EmberObject or can it be a parentless ES class?
• Can deprecations ({until: '3.0.0'}) be removed?
• Can the camelCase warning be removed?
• Can _logLookup be removed as per the comment?
• Can the commented-out code in container-debug-adapter.js be uncommented now that this has been resolved: allow all types, not just view to be resolved via namespace #80 ?

[chriskrycho]

@bwbuchanan somehow I totally missed this, and came back across it while flushing out my email this morning—will respond after the holiday!

[chriskrycho]

@bwbuchanan all right, sorry about the delay:

• Should the addon use ember-source as a devDependency to get access to the types for Resolver, FullName, Factory, KnownForTypeResult?

Yep, that's correct. You’ll also want to explicitly supply a peerDependencies entry for ember-source, and let people know that they cannot rely on the types from the package unless they’re on at least Ember 4.8. I’ll see about updating the DefinitelyTyped types as well so that we can also do an optional peerDependencies entry for the relevant @types packages so that this can work both ways.

• What is the public API of ember-resolver? Is it all of the methods and properties that are not underscore-prefixed?

The public API for our purposes actually just matches what we have on DefinitelyTyped—I happen to have had the same question when updating that for Ember v4 support!

• What would break by refactoring to use ES class syntax instead of EmberObject.extend()? Are there downstream consumers of ember-resolver that subclass it and depend on some particular quirks of EmberObject inheritance versus ES class inheritance?

A good question. I think we probably want to do that but separately from the types work, not least so that we can do it in a major release which makes it explicit that we're dropping support for any classic class dependencies on it. I’ll confirm this with @rwjblue and the rest of Framework folks; it may be a bit dicier since we haven’t yet deprecated classic classes in general—but I think it’s perfectly reasonable to tackle on packages like this; Glimmer Component already requires native classes, so it's mostly important that we have a clear deprecation cycle and use

• Does the resolver need to stay an instance of EmberObject or can it be a parentless ES class?

This is obviously related to the previous question. Purely from a mechanical POV, once classic class support is gone there’s zero reason to use EmberObject. The main thing here would be looking at how it’s instantiated on the Ember side, and making sure that if the Ember side registers it with the DI system and sets an owner for it (which I believe it does), we do the same, just with the native class.

• Can deprecations ({until: '3.0.0'}) be removed?

Heh, very much yes. Let's also keep that in a separate PR, but please do let's.

• Can the camelCase warning be removed?
• Can _logLookup be removed as per the comment?

I haven't looked at these specifically, but presumably—again, let’s land each of these in separate PRs, just for the sake of CHANGELOG generation and ease of bisecting if something goes amiss later.

• Can the commented-out code in container-debug-adapter.js be uncommented now that this has been resolved: allow all types, not just view to be resolved via namespace allow all types, not just view to be resolved via namespace #80 ?

I’m going to have to defer entirely to @rwjblue on this one.

[chriskrycho]

@bwbuchanan I added an ambient type definition at #823, just to keep things moving, but we would still very much welcome a full conversion!

[bwbuchanan]

@chriskrycho Thanks for the answers/clarifications above. I plan to make a draft PR in the next few days after resolving all typecheck/lint issues.

[bwbuchanan]

@chriskrycho

Currently blocked on this issue:

ember-source/types/preview/@ember/debug/container-debug-adapter.d.ts appears to be missing this line:

import Object from '@ember/object';

And this unfortunately breaks the TypeScriptified ember-resolver addon/resolvers/classic/container-debug-adapter.ts because TypeScript thinks that ContainerDebugAdapter is not an EmberObject and .exend() doesn't exist.

I will raise a bug against ember.

[kategengler]

cc @ef4 for the peerDeps question

[chriskrycho]

Thanks! Will see about getting that unblocked this afternoon!

[ef4]

I think it's fine to have a peerDep on ember-source. Especially since nobody can really use this package without already have that anyway, so it's not imposing anything new. It's just communicating to the package manager that ember-resolver needs to resolve the app's ember-source.

[bwbuchanan]

Thanks for the quick fix to the ember-source typings @chriskrycho!

My WIP branch for the TypeScript conversion is here: https://github.com/bwbuchanan/ember-resolver/tree/convert-to-typescript

My objectives:

• Where possible, just apply type annotations without refactoring or changing semantics.
• Avoid unsafe type assertions and use of any, making type assertions only where safe and necessary to make inferences that the TS compiler can't.
• Where necessary to change semantics to conform to type contracts, make the smallest change possible.
• Conform to @tsconfig/ember/tsconfig.json compiler options
• Conform to @typescript-eslint/eslint-recommended and @typescript-eslint/recommended

[bertdeblock]

I merged the linked PR, but I'm not sure if the issue itself should be closed? As it seems we want to do an actual conversion.

[chriskrycho]

Right, we should publish a release with that definition ASAP as it unlocks work across the rest of the ecosystem, and then @bwbuchanan’s PR can follow it without any particular urgency (though obviously we’ll want to switch over to using it as soon as we can!).

[bwbuchanan]

TypeScript conversion PR