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

Uninstall ember-classic-decorator #215

Closed
wants to merge 1 commit into from
Closed

Uninstall ember-classic-decorator #215

wants to merge 1 commit into from

Conversation

charlesfries
Copy link
Collaborator

ember-classic-decorator in its current state is not compatible with Ember 4 (the following PR will fix it but it's yet to be merged: emberjs/ember-classic-decorator#83)

I think we are able to get by without it, but I'm going to do some research into Ember Data and Ember Simple Auth to make sure. Opening the PR anyways.

public init(...args: unknown[]): void {
this._super(...args);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public constructor(...args: any[]) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried using the unknown[] type here but I can't then call super() with that unknown type.

@mikkopaderes
Copy link
Owner

mikkopaderes commented Dec 23, 2021

I can recall now why I used ember-classic-decorator, it's because if we use the init hook, the eslint will throw this error. But I believe the constructor way works now so we could proceed with this.

Copy link
Owner

@mikkopaderes mikkopaderes left a comment

Choose a reason for hiding this comment

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

Let me hold off on this change. I'm applying it in #217 as I want to fix all deprecations in 1 PR so that we could see if the CI build succeeds. Once builds are succeeding then we could start looking into the bugfix PRs so that CI builds for them also succeeds.

@mikkopaderes
Copy link
Owner

@charlesfries I've applied this in #217. Thanks for providing the fix!

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