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

More TS #19964

Merged
merged 5 commits into from
Feb 24, 2022
Merged

More TS #19964

merged 5 commits into from
Feb 24, 2022

Conversation

wagenet
Copy link
Member

@wagenet wagenet commented Feb 10, 2022

No description provided.

@wagenet
Copy link
Member Author

wagenet commented Feb 11, 2022

This will need to be cleaned up before merging.

@@ -77,7 +81,7 @@ const VALID_FULL_NAME_REGEXP = /^[^:]+:[^:]+$/;
*/
export default class Registry implements IRegistry {
readonly _failSet: Set<string>;
resolver: Resolver | (Resolve & NotResolver) | null;
resolver: Resolver | null;
Copy link
Member Author

@wagenet wagenet Feb 19, 2022

Choose a reason for hiding this comment

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

This seems like it is probably incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting how it works on line 64, are we sure it's not right? (It's certainly weird, and I don't know the runtime code here at all so I'll defer to you on the judgment call.)

resolver?: Resolver | (Resolve & NotResolver);

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably deserves a more careful look at least!

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Couple small tweaks.

@@ -3144,7 +3144,7 @@ Clearly, `component-a` has subscribed to `some-other-component`'s `action`. Prev
* CollectionView context is now its content
* Various enhancements to bound helpers: adds multiple property support to bound helpers, adds bind-able options hash properties, adds {{unbound}} helper support to render unbound form of helpers.
* Add App.inject
* Add Ember.EnumberableUtils.intersection
* Add Ember.EnumerableUtils.intersection
Copy link
Contributor

Choose a reason for hiding this comment

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

😂 Love it.

@@ -77,7 +81,7 @@ const VALID_FULL_NAME_REGEXP = /^[^:]+:[^:]+$/;
*/
export default class Registry implements IRegistry {
readonly _failSet: Set<string>;
resolver: Resolver | (Resolve & NotResolver) | null;
resolver: Resolver | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting how it works on line 64, are we sure it's not right? (It's certainly weird, and I don't know the runtime code here at all so I'll defer to you on the judgment call.)

resolver?: Resolver | (Resolve & NotResolver);

@public
*/
resolver: null,
declare resolver: Resolver;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given it's set in the constructor, shouldn't need the declare here.

Suggested change
declare resolver: Resolver;
resolver: Resolver;

if (!reason) return;

let withErrorThrown = reason as ReasonWithErrorThrown;
if (withErrorThrown?.errorThrown) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional chaining not needed here given the !reason check on line 42.

Suggested change
if (withErrorThrown?.errorThrown) {
if (withErrorThrown.errorThrown) {

Comment on lines 3 to 7
// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface ActionHandler {}
declare const ActionHandler: Mixin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will presumably need a rebase now that #19948 has also updated this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

import { Mixin } from '@ember/-internals/metal';

interface PromiseProxyMixin<T> {
reason: null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value is presumably null but this would make it so you can't use anything but null. From the docs, I think it should be unknown?

Suggested change
reason: null;
reason: unknown;

@chriskrycho chriskrycho merged commit 086460c into emberjs:master Feb 24, 2022
nevilm-lt pushed a commit to nevilm-lt/ember.js that referenced this pull request Apr 22, 2022
nevilm-lt pushed a commit to nevilm-lt/ember.js that referenced this pull request Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants