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

Convert codebase to use native classes #868

Merged
merged 1 commit into from
Apr 13, 2024
Merged

Conversation

SergeAstapov
Copy link
Contributor

Extract from #855 to keep it focused

@@ -1,8 +1,8 @@
import { inject as service } from '@ember/service';
import Route from '@ember/routing/route';

export default Route.extend({
exampleService: service(),
export default class extends Route {
Copy link
Collaborator

Choose a reason for hiding this comment

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

think the class should be named here

},
});
@action goToChineseVersion() {
this.transitionTo({ queryParams: { lang: 'Chinese' } });
Copy link
Collaborator

Choose a reason for hiding this comment

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

converting this in isolation of the 855 pr change? if yes then that pr will need need to recharge this line to make tests work with e-engines-router-service due to a known bug with qp only transitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@void-mAlex sorry I think I missed your point. This PR focuses solely on syntax change, without any functional changes.

Once we land this, we we would simply need to rebase #855, do all the necessary functional tweaks without noise of syntax conversion and land it with sole focus on v5 support via using e-engines-router-service

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to merge it before the #855?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to merge it before the #855?

the idea is to not have a mega pr that does everything

@void-mAlex sorry I think I missed your point. This PR focuses solely on syntax change, without any functional changes.

Once we land this, we we would simply need to rebase #855, do all the necessary functional tweaks without noise of syntax conversion and land it with sole focus on v5 support via using e-engines-router-service

my concern is that a decent amount of time will be wasted debugging tests again, which is fine, it's not too daunting, but tried leaving a breadcrumb to one conflict I know we'll have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried leaving a breadcrumb to one conflict I know we'll have

noted! will be glad to help

Copy link
Member

Choose a reason for hiding this comment

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

my concern is that a decent amount of time will be wasted debugging tests again

I agree! would be great to avoid back in forth

@villander
Copy link
Member

@SergeAstapov I don't think we need to fix it first before merging the #855 could you please clarify what you're trying to achieve here enriching the PR description?

@SergeAstapov SergeAstapov merged commit 785ff4b into master Apr 13, 2024
9 checks passed
@SergeAstapov SergeAstapov deleted the native-classes branch April 13, 2024 15:13
@github-actions github-actions bot mentioned this pull request Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants