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

Feature/2010 seo friendly urls dynroutes #2446

Merged

Conversation

Projects
None yet
4 participants
@pkarw
Copy link
Collaborator

commented Feb 16, 2019

Related issues

Related to #2401 - second approach

Short description and why it's useful

(describe in a few words what is this Pull Request changing and why it's useful)

Screenshots of visual changes before/after (if there are any)

(if you made any changes in the UI layer please provide before/after screenshots)

Screenshot of passed e2e tests (if you are using our standard setup as a backend)

(run yarn test:e2e and paste the results. If you are not using our standard backend setup or demo.vuestorefront.io you can ommit this step)

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

Contribution and currently important rules acceptance

@filrak

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

This one looks much better and less complicated at first glance! I still need time to dig deeper into it tomorrow to understand the details but overally it looks like the right way to do this :)

The only thing that concerns me is there might be better to put all of this stuff in one VS module. I guess if we want this feature to be optionally it's good to keep it in a separate threeshakable module so people that don't use it are not including the unused code into the bundle. It also helps very much with understanding and maintenanc since everything regarding this feature is in one place.

Even if we are adding something to the function directly (like config checks)

 computed: {
    productLink () {
      return ((this.$store.state.config.seo.useUrlDispatcher) ? 
        this.localizedDispatcherRoute({
          fullPath: this.$store.state.config.seo.useUrlDispatcher ? this.product.url_path : null,
          params: {
            childSku: this.product.sku === this.product.parentSku ? null : this.product.sku
          }
        })
      :
        this.localizedRoute({
          name: this.product.type_id + '-product',
          params: {
            parentSku: this.product.parentSku ? this.product.parentSku : this.product.sku,
            slug: this.product.slug,
            childSku: this.product.sku
          }
        })
      )   
    },  

maybe it's still better to keep this logic inside module and just import this function as a helper or something else.

With this the separation of concerns is kept and in case of lack of the module we are only gently warned about failed import instead of being forced to debug code and figure out which part is responsible for the module that we don't have?

So instead of above code it may look like:

import { CustomURLProductLink } from '@vue-storefront/core/modules/seo-priendly-url/helpers'
// later in code
 computed: {
    productLink: 
      CustomURLProductLink(this) :||
      // regular code if module is not working
 },  

I guess it may be a good general approach for such things in the project. Wdyt?

// Edited

@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 16, 2019

Thanks Filip :) Yea that might have sense however in that case url module always must be loaded it’s no longer optional. Let’s think this through. The only problem I’ve currently got with this approach is csr/ssr hydration issue :( no idea why :(

@filrak

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

maybe the hydration check is fired too early ( @patzick tried to make this check too and it didn't worked as intended ). afaik there was a library allowing to delay hydration (it's in some of recent vue newsletters)

Btw i figured out how to deal with removing this config checks and encapsulating all of the logic in the imported function.

import { CustomURLProductLink } from '@vue-storefront/core/modules/seo-friendly-url/helpers'
// later in code
 computed: {
    productLink: 
      this.$store.state.config.seo.useUrlDispatcher ? CustomURLProductLink(this) : 
      // regular code if module is not working
 },

we can just use OR logical operator since it always return the value of part that returned true instead of true so it can work like:

 computed: {
    productLink: CustomURLProductLink(this) || // regular code if module is not working
 },

if CustomURLProductLink will throw false it will return value of the second function so we basically need to check the config param inside function

CustomURLProductLink (context) {
 if (this.$store.state.config.seo.useUrlDispatcher) {
  // logic if the config param is present
 } else {
   return false
 }
}
@filrak

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

Yea that might have sense however in that case url module always must be loaded it’s no longer optional

why?

@pkarw pkarw referenced this pull request Feb 17, 2019

Closed

[WIP] PoC - multipart, dynamic URL rewrite #2401

1 of 5 tasks complete
@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2019

@filrak OK, i got it - so even module won't be loaded the helper could be used

@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2019

TODO:

  • code changes #2401 - for example parse-query
  • url helper in favor of "ifology"
  • tests
  • error handling tests
  • overall tests with the config.seo.urlDispatcher=false
@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 17, 2019

FIXED:

  • SSR support,
  • references to config.seo.useUrlDispatcher limited just to url module

TODO:

  • multistore routing
  • multistore links
@patzick
Copy link
Collaborator

left a comment

Pretty huge PR :)
i've made ~half of it and i'll finish it tomorrow, but i give what i have for now

(Note to myself: finished on core/modules/catalog/store/product/actions.ts)

Show resolved Hide resolved core/app.ts Outdated
Show resolved Hide resolved core/build/webpack.base.config.ts
Show resolved Hide resolved core/client-entry.ts Outdated
Show resolved Hide resolved core/client-entry.ts Outdated
Show resolved Hide resolved core/helpers/index.ts Outdated
Show resolved Hide resolved core/lib/logger.ts Outdated
Show resolved Hide resolved core/lib/multistore.ts Outdated
Show resolved Hide resolved core/lib/multistore.ts Outdated
Show resolved Hide resolved core/modules/catalog/store/category/mutations.ts Outdated
Show resolved Hide resolved core/modules/catalog/store/category/mutations.ts Outdated

@pkarw pkarw referenced this pull request Feb 20, 2019

Closed

Unified SSR checking way #2468

Update core/lib/multistore.ts
Co-Authored-By: pkarw <pkarwatka@divante.pl>

filrak and others added some commits Feb 26, 2019

Update core/modules/url/router/beforeEach.ts
Co-Authored-By: pkarw <pkarwatka@divante.pl>
Update core/modules/url/router/beforeEach.ts
Co-Authored-By: pkarw <pkarwatka@divante.pl>
@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2019

OK, It's ALL DONE!

82 comments; 58 files modified, 59 commits -> wow it became one of the biggest CRs processed so far haha :)

@pkarw pkarw requested a review from filrak Feb 26, 2019

@filrak

filrak approved these changes Feb 26, 2019

export const SET_USERS = 'TEMPLATE/SET_USERS'
export const ADD_USER = 'TEMPLATE/SET_USER'

This comment has been minimized.

Copy link
@filrak

filrak Feb 26, 2019

Collaborator

Thanks for correcting!

pkarw added some commits Mar 1, 2019

@pkarw pkarw added this to the 1.9-rc milestone Mar 1, 2019

@patzick

patzick approved these changes Mar 3, 2019

Copy link
Collaborator

left a comment

Okay, i think we can merge this now.

@pkarw please apply just suggestions and resolve conflicts - nice job here! This is gonna be a cool feature!

Show resolved Hide resolved CHANGELOG.md
Show resolved Hide resolved CHANGELOG.md Outdated
Show resolved Hide resolved core/app.ts Outdated

patzick and others added some commits Mar 4, 2019

Update CHANGELOG.md
Co-Authored-By: pkarw <pkarwatka@divante.pl>
Update CHANGELOG.md
Co-Authored-By: pkarw <pkarwatka@divante.pl>
Update core/app.ts
Co-Authored-By: pkarw <pkarwatka@divante.pl>
@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

@patzick OK, done!

Update core/modules/catalog/store/product/actions.ts
Co-Authored-By: pkarw <pkarwatka@divante.pl>
@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

Thanks, done

@filrak filrak merged commit d7a2866 into DivanteLtd:develop Mar 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pkarw pkarw referenced this pull request Mar 14, 2019

Closed

Add support for url_path #18

@Zyles

This comment has been minimized.

Copy link

commented Mar 20, 2019

Thanks for working on this. We want to go full headless but we can't until we know a storefont framework that can support the same URLs that Magento 2 itself generates. Because we don't want /p/ /c/ etc. in URLs or anything else like identifiers. Because we don't want to 301 redirect thousands of existing URLs.

Will this new update support product urls with "/my-product.html" and "/my-category/sub-category/" style? We use both, ".html" for products and "/" suffix for category/page.

Does it also support language code in url? "mydomain.com/fr/my-product.html" ?

Thanks.

@pkarw

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 21, 2019

@Zyles correct. With this feature - starting from 1.9.0-rc1 you'll be able to set the URL structure of your choice just using the product.url_path and category.url_path fields

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.