Future public @ember/engine types by wagenet · Pull Request #19923 · emberjs/ember.js

wagenet · 2022-01-26T18:54:46Z

No description provided.

chriskrycho

Will finish this review tomorrow, but WIP feedback!

chriskrycho · 2022-02-09T00:25:54Z

packages/@ember/-internals/container/lib/container.ts

   @return {any}
   */
-  lookup(fullName: string, options: LookupOptions): any {
+  lookup<T>(fullName: string, options?: TypeOptions): T | undefined {


This should actually return unknown, since it's impossible to infer from the arguments what the return type T would actually be.

Suggested change

lookup<T>(fullName: string, options?: TypeOptions): T | undefined {

lookup(fullName: string, options?: TypeOptions): unknown {

I know this matches the other implementations, but we should take care to move toward rather than away from safety with this work where our hands are not already tied in some way.

I expect you introduced this because there is client code which is relying on being able to freely cast because of the previous any return type?

This makes sense. I’ll give it a second look tomorrow.

chriskrycho · 2022-02-09T00:32:35Z

packages/@ember/-internals/container/lib/registry.ts

+  readonly registrations: { [key: string]: object | undefined };
+  _localLookupCache: { [key: string]: object | undefined };
+  readonly _normalizeCache: { [key: string]: string | undefined };
+  readonly _options: { [key: string]: TypeOptions | undefined };
+  readonly _resolveCache: { [key: string]: object | undefined };
+  readonly _typeOptions: { [key: string]: TypeOptions | undefined };


Can we just use Record<string, TypeOptions> etc. here?

It looks like this as well as the in check below are both because we don't yet have noUncheckedIndexAccess enabled, so we're sort of pushing that safety in manually?

We do have noUncheckedIndexAccess now. I’ll revisit this.

chriskrycho · 2022-02-09T00:34:25Z

packages/@ember/-internals/container/lib/registry.ts

  getOptions(fullName: string): TypeOptions | undefined {
    let normalizedName = this.normalize(fullName);
-    let options = this._options[normalizedName];
+    let options = normalizedName in this._options ? this._options[normalizedName] : undefined;


Hmm. This shouldn't be complaining about the access. 🤔 (See playground.) Since it's a string index, and given the type you wrote, it should correctly resolve it in either the Record or the index type form. What error was TS giving you here?

Not sure what was happening before but it seems ok now.

chriskrycho · 2022-02-09T00:36:09Z

packages/@ember/-internals/glimmer/lib/resolver.ts

-  owner: Owner,
-  options?: LookupOptions
-): Option<Factory<{}, {}>> {
+function componentFor(name: string, owner: Owner): Option<Factory<{}, {}>> {


This looks like an unrelated-to-types change – is this a dead code path or something?

The type was incorrect. factoryFor doesn't take options.

Glad we caught it by doing this, then. Thanks! 😃

chriskrycho · 2022-02-09T00:37:15Z

packages/@ember/-internals/owner/index.ts

-  factoryFor<T, C>(fullName: string, options?: LookupOptions): Factory<T, C> | undefined;
-  register<T, C>(fullName: string, factory: Factory<T, C>, options?: LookupOptions): void;
-  hasRegistration(name: string, options?: LookupOptions): boolean;
+  lookup<T>(fullName: string, options?: TypeOptions): T | undefined;


As before, I'm fine if we leave this for now, but we should start working to eliminate it. 😩

I'll fix it now.

chriskrycho · 2022-02-09T00:38:02Z

packages/@ember/-internals/runtime/lib/mixins/container_proxy.d.ts

+}
+declare const ContainerProxy: Mixin;
+
+export { ContainerProxy as default };


I believe the normal export default form should work; does it not for some reason?

Suggested change

export { ContainerProxy as default };

export default ContainerProxy;

chriskrycho · 2022-02-09T00:38:13Z

packages/@ember/-internals/runtime/lib/mixins/registry_proxy.d.ts

+}
+declare const RegistryProxyMixin: Mixin;
+
+export { RegistryProxyMixin as default };


Suggested change

export { RegistryProxyMixin as default };

export default RegistryProxyMixin;

chriskrycho

Guide:

Mark	Meaning
?	informational, non-blocking
~	not great, non-blocking
X	blocking

Spoilers: no X, so .

chriskrycho · 2022-02-10T22:54:03Z

packages/@ember/-internals/glimmer/lib/component-managers/curly.ts

    if (layout === undefined) {
      if (layoutName !== undefined) {
-        let _factory = owner.lookup<TemplateFactory>(`template:${layoutName}`);
+        let _factory = owner.lookup(`template:${layoutName}`) as TemplateFactory;


?: Is this cast safe? If so, why? (I see that it was already here.)

I don't know that it's safe, but it's how it was before so it probably is. I figure having as is safer (as in we know we're doing something potentially risky) than the way it was before where it wasn't clear that there was risk!

chriskrycho · 2022-02-10T22:54:43Z

packages/@ember/-internals/glimmer/lib/component.ts

+        if ((owner.lookup('-environment:main') as Environment)!.isInteractive) {
+          this.__dispatcher = owner.lookup('event_dispatcher:main') as EventDispatcher;


~: Would be good to follow on with dev-time assertions instead of casting.

chriskrycho · 2022-02-10T22:55:09Z

packages/@ember/-internals/glimmer/lib/modifiers/action.ts


  ensureEventSetup(actionState: ActionState): void {
-    let dispatcher = actionState.owner.lookup<EventDispatcher>('event_dispatcher:main');
+    let dispatcher = actionState.owner.lookup('event_dispatcher:main') as EventDispatcher;


?/~: as above re: casting and safety

chriskrycho · 2022-02-10T22:55:50Z

packages/@ember/-internals/glimmer/lib/resolver.ts

  let templateFullName = `template:components/${name}`;

-  return owner.lookup(templateFullName, options) || null;
+  return (owner.lookup(templateFullName, options) as Template) || null;


~: Could use a safety comment here in the future.

Yeah, again I don't know that this is actually safe!

chriskrycho · 2022-02-10T22:57:01Z

packages/@ember/-internals/glimmer/lib/resolver.ts

+    const factory = owner.factoryFor(`helper:${name}`) as
+      | Factory<SimpleHelper, HelperFactory<SimpleHelper>>
+      | Factory<HelperInstance, HelperFactory<HelperInstance>>;


?/~: as above on the cast.

Yup, unsure this is safe, but it seems like it must be.

chriskrycho · 2022-02-10T23:02:40Z

packages/@ember/-internals/routing/lib/system/router.ts

    this.namespace = owner.lookup('application:main');

-    let bucketCache: BucketCache | undefined = owner.lookup(P`-bucket-cache:main`);
+    let bucketCache = owner.lookup(P`-bucket-cache:main`) as BucketCache | undefined;


~: as throughout.

chriskrycho · 2022-02-10T23:03:09Z

packages/@ember/-internals/views/lib/system/utils.ts

 */
 export function getRootViews(owner: Owner): View[] {
-  let registry = owner.lookup<Dict<View>>('-view-registry:main')!;
+  let registry = owner.lookup('-view-registry:main') as Dict<View>;


~: as throughout, for reference.

chriskrycho · 2022-02-10T23:03:52Z

packages/@ember/application/type-tests/instance.test.ts

 ).toEqualTypeOf<void>();

-expectTypeOf(instance.lookup<Store>('service:store')).toEqualTypeOf<Store | undefined>();
+expectTypeOf(instance.lookup('service:store')).toEqualTypeOf<unknown>();


If we add that registry, these will (happily!) flag it up for us as a case where it no longer equals that type. 🎉

chriskrycho · 2022-02-10T23:05:37Z

packages/@ember/debug/lib/capture-render-tree.ts

 */
 export default function captureRenderTree(app: Owner): CapturedRenderNode[] {
-  let renderer = expect(app.lookup<Renderer>('renderer:-dom'), `BUG: owner is missing renderer`);
+  let renderer = expect(app.lookup('renderer:-dom') as Renderer, `BUG: owner is missing renderer`);


~: could use a better assertion to check that it's not just present but is the right/expected thing.

chriskrycho · 2022-02-10T23:05:52Z

packages/@ember/engine/lib/engine-parent.ts

  @private
 */
-export function getEngineParent(engine) {
+export function getEngineParent(engine: EngineInstance): EngineInstance | undefined {


wagenet force-pushed the ember-engine-types branch 2 times, most recently from ea75656 to dd914f6 Compare February 2, 2022 19:53

wagenet force-pushed the ember-engine-types branch 2 times, most recently from e34ab4a to cd198dd Compare February 7, 2022 18:17

wagenet mentioned this pull request Feb 7, 2022

[QUEST] Future Public TypeScript Types #19946

Closed

chriskrycho suggested changes Feb 9, 2022

View reviewed changes

wagenet force-pushed the ember-engine-types branch from cd198dd to 47ff27b Compare February 9, 2022 01:59

wagenet added 3 commits February 10, 2022 14:31

Future public @ember/engine types

d268677

Resolve unsafe owner methods

d4c40f1

Fix failing tests

b97e01a

wagenet force-pushed the ember-engine-types branch from 47ff27b to b97e01a Compare February 10, 2022 22:37

chriskrycho approved these changes Feb 10, 2022

View reviewed changes

wagenet merged commit a94cf49 into emberjs:master Feb 10, 2022

wagenet deleted the ember-engine-types branch February 11, 2022 01:37

	lookup<T>(fullName: string, options?: TypeOptions): T \| undefined {
	lookup(fullName: string, options?: TypeOptions): unknown {

	export { ContainerProxy as default };
	export default ContainerProxy;

	export { RegistryProxyMixin as default };
	export default RegistryProxyMixin;

		if ((owner.lookup('-environment:main') as Environment)!.isInteractive) {
		this.__dispatcher = owner.lookup('event_dispatcher:main') as EventDispatcher;

Uh oh!

Conversation

wagenet commented Jan 26, 2022

Uh oh!

chriskrycho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriskrycho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants