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.

Sign up for GitHub

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

Jump to bottom

[FEAT] Upstream manager infrastructure #19267

Merged
merged 1 commit into from
Nov 13, 2020
Merged

[FEAT] Upstream manager infrastructure #19267

merged 1 commit into from
Nov 13, 2020

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Nov 12, 2020 •
edited
Loading

rwjblue
rwjblue requested changes Nov 13, 2020
View reviewed changes
packages/@ember/-internals/glimmer/lib/component-managers/internal.ts
@@ -41,37 +41,31 @@ export interface InternalComponentState {
}

export class InternalComponentDefinition
Copy link
Member

Choose a reason for hiding this comment

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

Calling these interfaces "internal" now seems odd (since glimmer-vm also has a set of things it calls "internal"). Not sure what the best name here would be, but something like EmberInternal* seems better than the current confusion (at least my confusion 😜 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something that I plan on addressing in the next set of PRs, which will upstream most of the component manager logic. Part of that will be unifying internal and public component managers so they can both be used with setComponentManager, and in general use the same infrastructure. This way for instance, curly components will be able to be used in strict mode, since their internal manager will be associated with setComponentTemplate.

As part of that work, this manager will likely be renamed InputComponentManager or something like that. I could rename it as part of this PR I suppose, it's just a bit weird since the resolver in Ember still has a notion of "internal", and I didn't want to get rid of that all at once either.

packages/@ember/-internals/glimmer/lib/helper.ts Outdated
@@ -197,7 +197,7 @@ class ClassicHelperManager implements HelperManager<ClassicHelperStateBucket> {
}
}

setHelperManager((owner) => new ClassicHelperManager(owner), Helper);
setHelperManager((owner: Owner | undefined) => new ClassicHelperManager(owner!), Helper);
Copy link
Member

Choose a reason for hiding this comment

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

Seems very suspect to update the signature here to allow undefined and to use owner!.

packages/@ember/-internals/glimmer/lib/helpers/custom.ts
@@ -430,6 +415,8 @@ export function invokeHelper(
manager
);

assert('Invoke helper does not support internal helpers yet', !isInternalHelper(manager));
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a new addition? Should we consider backporting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new addition, it is to allow us to associate internal helpers directly with a function via setHelperManager. This way we can make all of the built-in helpers work in strict mode, when you import them.

Currently, those internal helpers get resolved above, and we don't even associate any internal helpers directly with setHelperManager, so this assertion should never be triggered. I added it to satisfy the type constraints.

packages/@ember/-internals/glimmer/lib/modifiers/action.ts
VMArguments,
} from '@glimmer/interfaces';
import { isInvokableRef, updateRef, valueForRef } from '@glimmer/reference';
import { registerDestructor } from '@glimmer/runtime';
import { createUpdatableTag } from '@glimmer/validator';
import { createUpdatableTag, UpdatableTag } from '@glimmer/validator';
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how you feel, but I'd love it if type only imports were done separately....

Suggested change
import { createUpdatableTag, UpdatableTag } from '@glimmer/validator';
import { createUpdatableTag } from '@glimmer/validator';
import type { UpdatableTag } from '@glimmer/validator';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mainly add imports via autocomplete (Command + . in VSCode), so if we can set it up so that it does type imports for types, I'm happy with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also get a linting error for this, so I'm going to leave it as is for now

packages/@ember/-internals/glimmer/lib/resolver.ts Outdated
@@ -361,6 +370,11 @@ export default class RuntimeResolverImpl implements RuntimeResolver<Owner> {
manager === SIMPLE_CLASSIC_HELPER_MANAGER
);

assert(
'internal helpers are not supported via `getHelperManager` yet',
Copy link
Member

Choose a reason for hiding this comment

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

should include more information here, for example it should include the debug render tree stack and the name of the helper they asked for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The debug render tree stack will be included in general, because we throw an error. There's no reason to manually add the stack, it'll always be logged regardless. Will add the name.

packages/@ember/-internals/glimmer/lib/resolver.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/glimmer/lib/resolver.ts Outdated Show resolved Hide resolved
packages/@ember/-internals/glimmer/tests/integration/custom-component-manager-test.js Show resolved Hide resolved
packages/@ember/debug/index.ts
@@ -21,9 +21,7 @@ export type DebugFunctionType =
| 'runInDebug'
| 'deprecateFunc';

export type AssertFunc =
| ((desc: string) => never)
Copy link
Member

Choose a reason for hiding this comment

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

| ((desc: string) => never)

What was this overload even for? Seems weird...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea, but it was messing with the asserts from TS so I removed, and everything seems to work 🤷

@pzuraq pzuraq mentioned this pull request Nov 13, 2020
Merged
@pzuraq pzuraq force-pushed the upstream/manager-infrastructure branch from 4255b76 to 420fa9a Compare November 13, 2020 18:14
@pzuraq pzuraq force-pushed the upstream/manager-infrastructure branch from 420fa9a to 615f8e4 Compare November 13, 2020 21:53
@pzuraq pzuraq marked this pull request as ready for review November 13, 2020 22:15
@pzuraq pzuraq merged commit 0f3266c into master Nov 13, 2020
@pzuraq pzuraq deleted the upstream/manager-infrastructure branch November 13, 2020 22:16
@locks locks mentioned this pull request Jan 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pzuraq @rwjblue