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

Type error when using useResource #48

Closed
nightire opened this issue Jun 29, 2021 · 19 comments · Fixed by #49 or #168
Closed

Type error when using useResource #48

nightire opened this issue Jun 29, 2021 · 19 comments · Fixed by #49 or #168
Labels

Comments

@nightire
Copy link

nightire commented Jun 29, 2021

image

First of all, I can not write a failing test because in test cases, we instantiate the resource class directly, but the type error is caused by useResource utility function.

Then I spend a little time investigating what's going on with useResource, here when describing the arguments of Constructable, I found:

export interface Constructable<T = unknown> {
    new (...args: unknown[]): T; // <- `unknown` causes the type error
}

If I change it to any[] or more precisely (Positional | Named)[], the problem will be solved, at least for my case. I'm not sure if this fix is correct, @NullVoxPopuli how do you think?

@NullVoxPopuli
Copy link
Owner

Ah! That could be it. Yeah, I'd be happy with any there. It more or less is the same in this case anyway

@NullVoxPopuli
Copy link
Owner

NullVoxPopuli commented Jun 29, 2021

does Positional<[Field[]]> work?

upon closer inspection, it looks like you are passing an array of fields, but the type of the positional params is 0..n fields, which would be invoked as useResource(this, PreviewGrid, () => [...this.selectedFields[) (note the spread)

tldr:
Positional<Field[]>: useResource(this, PreviewGrid, () => [...this.selectedFields[)
Positional<[Field[]]>: useResource(this, PreviewGrid, () => [this.selectedFields[)

@nightire
Copy link
Author

does Positional<[Field[]]> work?

Unfortunately Positional<[Field[]]> still causes the same type error.

@NullVoxPopuli
Copy link
Owner

I made this demo, of what I thought was the issue, and the type checking passes: #49

Does this help?

@nightire
Copy link
Author

I made this demo, of what I thought was the issue, and the type checking passes: #49

Does this help?

Nope.

I'm starting to think maybe it's my side's problem, maybe I need to try in a new ember app instead.

@NullVoxPopuli
Copy link
Owner

Lemme know how it goes!

@nightire
Copy link
Author

image

@NullVoxPopuli I can confirm the types are correct in a new ember app, now I need to figure out the problem of my side. Thank you anyway.

@nightire nightire reopened this Jun 30, 2021
@nightire
Copy link
Author

nightire commented Jun 30, 2021

@NullVoxPopuli I reopen this issue because I found the root cause; I got this error before because I set "strict": true in tsconfig.json, ember-cli-typescript doesn't use this flag by default, but our company follows a more strict policy.

Since "strict": true means to enable a wide range of type checking, I try to toggle them one by one, and I'm pretty sure "strictFunctionTypes": true will lead to this error. According to its documentation, I think it does make sense:

When enabled, this flag causes functions parameters to be checked more correctly.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2021

🎉 This issue has been resolved in version 1.3.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@NullVoxPopuli
Copy link
Owner

@nightire do you happen to have a reproduction repo?

@nightire
Copy link
Author

nightire commented Sep 3, 2021

@NullVoxPopuli Sorry for the delay, I've got caught up in something else this last month.

Here is a reproduction: https://github.com/nightire/ember-resources-type-issue/tree/main#ember-resources-type-issue, hope it can help you.

NullVoxPopuli added a commit that referenced this issue Sep 5, 2021
NullVoxPopuli added a commit that referenced this issue Sep 5, 2021
github-actions bot pushed a commit that referenced this issue Sep 5, 2021
## [3.2.1](v3.2.0...v3.2.1) (2021-09-05)

### Bug Fixes

* **types:** useResource types now reflect that you *can* make non-reactive resources ([9059c90](9059c90)), closes [#48](#48)
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2021

🎉 This issue has been resolved in version 3.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@NullVoxPopuli
Copy link
Owner

@nightire can you try this latest release?, 3.2.1?

@nightire
Copy link
Author

nightire commented Sep 7, 2021

@nightire can you try this latest release?, 3.2.1?

Unfortunately, the error remains. I can live with "strictFunctionTypes": false for now.

@NullVoxPopuli
Copy link
Owner

Does the error in your reproduction go away?
What's the reproduction missing? 🤔

@NullVoxPopuli NullVoxPopuli reopened this Sep 7, 2021
@nightire
Copy link
Author

nightire commented Sep 8, 2021

Does the error in your reproduction go away?
What's the reproduction missing? 🤔

I have upgraded the ember-resources in my reproduction, and the error remains.

This repro is a brand new v3.28.0 ember application with the latest ember-cli-typescript and ember-resources, nothing else.

BTW, this small change will solve the error as I mentioned at the beginning of this issue. Is it an acceptable solution? I'm not an expert on typescript, just ask for curiosity.

export interface Constructable<T = unknown> {
-  new (...args: unknown[]): T;
+  new (...args: any[]): T;
}

@lougreenwood
Copy link

I also just hit this issue after enabling strict TS mode.

@nightire
Copy link
Author

I also just hit this issue after enabling strict TS mode.

You can use strict mode with strictFunctionType off:

  "strict": true,
  "strictFunctionTypes": false,

I found this setting is not that important as we thought it would be, according to the official document:

During the development of this feature, we discovered a large number of inherently unsafe class hierarchies, including some in the DOM. Because of this, the setting only applies to functions written in function syntax, not to those in method syntax

@NullVoxPopuli
Copy link
Owner

fixed via... deprecation 😅

better alternative landed in v4.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants