Skip to content

refactor!: refactored useUrlResolver to use the route query #1078

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
Merged
merged 2 commits into from
Jun 3, 2022
Merged

refactor!: refactored useUrlResolver to use the route query #1078

merged 2 commits into from
Jun 3, 2022

Conversation

@Frodigo
Copy link
Contributor

@Frodigo Frodigo commented May 30, 2022 •
edited
Loading

Description

THe urlResolver Magento query is deprecated on Magento API side. This PR:

  • adds a new route query to the VSF API Client
  • updates the useUrlResolver composable to use the new route query instead of depracated urlResolver query.

Related Issue

Motivation and Context

Refactorization

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Frodigo Frodigo force-pushed the feature/M2-528-refactor-depracated-url-resolver-query-to-use-the-route-query branch from 262f6ac to 4db7d51 Compare May 30, 2022 12:54
@Frodigo Frodigo added this to the 1.0.0-rc.9 milestone May 30, 2022
@Frodigo Frodigo marked this pull request as ready for review May 30, 2022 13:00
@Frodigo Frodigo changed the title refactor: m2-528. refactored useUrlResolver to use the route query refactor: refactored useUrlResolver to use the route query May 30, 2022
sethidden
sethidden reviewed May 30, 2022
View reviewed changes
sethidden
sethidden reviewed May 30, 2022
View reviewed changes
sethidden
sethidden reviewed May 30, 2022
View reviewed changes
packages/theme/composables/useUrlResolver/index.ts Outdated
const search = async (): Promise<ROUTE_TYPE> => {
loading.value = true;
let results: EntityUrl = {};
let results = null;
Copy link
Contributor

@sethidden sethidden May 30, 2022

Choose a reason for hiding this comment

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

Suggested change
let results = null;
let results : EntityUrl | null = null;

Don't remove the type assertion. Right now the type of results is "any"

Copy link
Contributor

@sethidden sethidden May 30, 2022

Choose a reason for hiding this comment

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

If EntityUrl doesn't work now please use something else that fits

Copy link
Contributor Author

@Frodigo Frodigo May 30, 2022

Choose a reason for hiding this comment

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

fixed, but I had a problem with wrong generic type and I needed to add @ts-ignore in one line below.

Copy link
Contributor

@sethidden sethidden May 31, 2022

Choose a reason for hiding this comment

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

Wouldn't count ts-ignore as a fix unless there's some third party TS mistake :P Hopefully Bart's comment about removing the generic should help

Copy link
Contributor Author

@Frodigo Frodigo May 31, 2022

Choose a reason for hiding this comment

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

After chatting with Bartek, the generic type need to stay here. I tried to fix ts-ignore but without success. Could you check out my branch and check that by yourself?

Copy link
Collaborator

@bartoszherba bartoszherba Jun 1, 2022

Choose a reason for hiding this comment

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

@Frodigo I just committed a working proposition. Check it out and subscribe my channel ;)

sethidden
sethidden reviewed May 30, 2022
View reviewed changes
sethidden
sethidden reviewed May 30, 2022
View reviewed changes
sethidden
sethidden requested changes May 30, 2022
View reviewed changes
Copy link
Contributor

@sethidden sethidden left a comment

Choose a reason for hiding this comment

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

Left some comments

@Frodigo Frodigo force-pushed the feature/M2-528-refactor-depracated-url-resolver-query-to-use-the-route-query branch from 4db7d51 to 622cb60 Compare May 30, 2022 18:41
@Frodigo Frodigo requested a review from sethidden May 30, 2022 18:41
bartoszherba
bartoszherba reviewed May 31, 2022
View reviewed changes
bartoszherba
bartoszherba reviewed May 31, 2022
View reviewed changes
bartoszherba
bartoszherba requested changes May 31, 2022
View reviewed changes
Copy link
Collaborator

@bartoszherba bartoszherba left a comment

Choose a reason for hiding this comment

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

I think that the Router type must be removed.

sethidden
sethidden requested changes May 31, 2022
View reviewed changes
Copy link
Contributor

@sethidden sethidden left a comment

Choose a reason for hiding this comment

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

Can we please fix the ts-ignore? The error should be fixable, for example with Bartek's suggestions. If not, we can hop on a call

bartoszherba
bartoszherba reviewed May 31, 2022
View reviewed changes
@Frodigo Frodigo force-pushed the feature/M2-528-refactor-depracated-url-resolver-query-to-use-the-route-query branch from 622cb60 to b15487d Compare May 31, 2022 07:45
@Frodigo Frodigo requested review from bartoszherba and sethidden May 31, 2022 07:46
@bartoszherba bartoszherba force-pushed the feature/M2-528-refactor-depracated-url-resolver-query-to-use-the-route-query branch from 3a44eb0 to efd579e Compare June 1, 2022 12:25
sethidden
sethidden previously approved these changes Jun 1, 2022
View reviewed changes
@bartoszherba bartoszherba requested a review from sethidden June 1, 2022 12:39
@Frodigo Frodigo force-pushed the feature/M2-528-refactor-depracated-url-resolver-query-to-use-the-route-query branch from efd579e to 70fcccd Compare June 3, 2022 06:59
@Frodigo Frodigo merged commit 45f9707 into develop Jun 3, 2022
@Frodigo Frodigo deleted the feature/M2-528-refactor-depracated-url-resolver-query-to-use-the-route-query branch June 3, 2022 07:09
@Frodigo Frodigo changed the title refactor: refactored useUrlResolver to use the route query refactor!: refactored useUrlResolver to use the route query Jun 10, 2022
@Frodigo Frodigo mentioned this pull request Jun 10, 2022
Merged
@Frodigo Frodigo mentioned this pull request Jun 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bartoszherba bartoszherba bartoszherba approved these changes

@sethidden sethidden Awaiting requested review from sethidden

Assignees

@Frodigo Frodigo

Labels

None yet

Projects

None yet

Milestone

1.0.0-rc.9

Development

Successfully merging this pull request may close these issues.

4 participants

@Frodigo @sethidden @bartoszherba