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

pkarw · 2019-02-08T23:08:42Z

Related issues

#2010

Short description and why it's useful

This is proof of the concept of dynamic URL mapper. The challenge is that we need to support the fully customizable URLs - that don't have any specific elements pointing out if that's the Product, Category either CMS page.

Unfortunately, we CAN NOT use the vue-router guards as there is no way to rewrite URL (display different route/different component under the same URL). The only option was to create a page which is including the proper subcomponents.

We should decide if that's the way we would like to go. If so, we should then decide if this module should be placed in core or rather src - to let the users modify it when needed. After all - I think it should be src. The Dispatcher can vary from project to project - mostly because some custom page types added.

If we decided that this is the way to go -> I'll add the rest of the business logic.

Other options for URL dispatching mechanism:

kind of refactor Product, Category, CmsPage pages - but in the end it comes up to kind of "multipurpose" component which is breaking the SR Principle
Express.js level routing - but it doesn't solve the CSR routing isses (the URLs will differ CSR/SSR which is not good).
??

To test this out - please enter the http://localhost:3000/fake/product/url.html in the browser - and You'll get the Gwyn Endurance product page.

I've got an idea that the url/registerMapping will be called inside the category/list and product/list matching the products and categories with their custom URLs (product.url_path + product.url_key and category.url_path + category.url_key). If some random URL will be called (that's been not yet mapped) then the url/mappingFallback action will be called.

This action will be a subject to override by user - but be default it will be calling first product/single and then category/single to check if specified URL is a product or category URL.
Once mapped URL will be stored in localStorage + url.state (because we'll be getting the prefetched mappings from SSR renderer within this initial state)

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

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

I read and followed contribution rules
I read the TypeScript Action Plan and adjusted my PR according to it
I read about Vue Storefront Modules and I am aware that every new feature should be a module

pkarw · 2019-02-08T23:12:42Z

The second phase can - adding the /api/url/map - that will be doing the Category/Product/CMS distinction Server Side in the vue-storefront-api. This will save some % of users the second call (if URL is not product then second call for category will be made in mappingFallback - in the MVP)

pkarw · 2019-02-08T23:15:42Z

Reference materials:

Is it possible change url but dont render component by vue-router now? vuejs/vue-router#988
Allow absolute path change without destroying rendered components (Twitter-style modals) vuejs/vue-router#703
Rewriting URLs vuejs/vue-router#1858
Mapping dynamic route parameter values to aliases for use in the url vuejs/vue-router#1283

pkarw · 2019-02-08T23:19:02Z

Notes:

rootStore.state.config.products.useShortCatalogUrls should be set to true
product.slug and category.slug are used for generating the links so we could just change the way these links are created to get the proper, absolute link (product.slug should be like product.url_path + product.url_key for example - and the same for categories)

https://github.com/pkarw/vue-storefront/blob/4fb53e5009e8b83ef2901502fe150e403b63d920/core/lib/search/adapter/api/searchAdapter.ts#L97

filrak · 2019-02-09T14:44:46Z

In my opinion we should just change the way we are fetching data inside stores. As far as i understand the problem is that we need to pass additional params to determine what route we are fetching so why we can't just add proper keys in the vue-storefront-api that are directly mapping to appropiate product. With this we will avoid all this additional complexity here that is really hard to understand.

By having this we can just use plain redirects but based on route.name instead of route.path so no additional identifiers like /c/ or /p/ etc.

WIth this param the redirection flow will look like this:

user enters /some url.
We are deciding if this is a category or product or anything else based on the unique key provided as route param /:uniquekey and redircting to proper route based on it's route.name. The important part is all routes have path format as followe; /:uniquekey and are just redirecting to other routes based on name property

Since routes are resolved top to bottom it should just be the first route after all static ones:

const router = new VueRouter({
  routes: [
    // static routes like /about, ./whatever
    { path: '/:key', redirect: to => {
      const type =  checkKeyType(key)  // check if to.params.key is product/category/what ever and set everything needed in vuex 
      next({ name: type, path: '/' + key })
    }},
    // above function will redirect to one of these two:
    { name: 'category', path: '/:categoryId' component: Category },
    { name: 'product', path: '/:productId', component: Product }
  ]
})

This mechanism can be used even without having this unique keys. We just need a helper that can determine what type of entity it is and voila - we have one helper instead of full module and everything is kept inside router config so full transparency and separation of concerns. We also avoid hacks with asyncData, additional dynamic components etc

filrak

It can be extremely simplified

pkarw · 2019-02-09T16:59:55Z

Filip I was considering these options:

we cant list all the URL’s to create aliases as for shops with like 100k ski (typical fashion) it will be roughly in megabytes
we must use ‘*’ to have multi part urls: /:uniquekey won’t handle “/category1/category2”
we cant use “next()” - it’s redirect and there is no way to keep the original url - this was my initial idea and I spent 2.5h testing it out yesterday without any success

pkarw · 2019-02-09T17:06:50Z

So my approach is mixed of what you’ve proposed (url/mappingFallback and url/mapUrl is this fallback to determine which route is it) adapted to make no redirects. Users can override these actions with their modules to have different mapping logic

I plan to make it simpler and more elegant and even more flexible with async components: https://vuejs.org/v2/guide/components-dynamic-async.html

So it probably won’t by a single file component (I mean url dispatcher) but just JS function + Product, Category, ... will be loaded using import().

With this urldispatcher approach - which Ian pretty standard for any CMs on the market (including Magento, Drupal and others) - having a Front Controller / Dispatcher design pattern implemented - we will have the flexibility of adding the redirects support when products are updated etc

We could then also include a support for the Magento url resolver: #2010 (comment)

Basically - my approach isn’t any fresh idea, it’s just kind of implementation for Magentos resolver in Vue.js :)

pkarw · 2019-02-09T17:16:04Z

Vue.js should have not only “next()” but also “router.forward()” forwarding the request flow to different route without switching the url. This is reason we can’t use this elegant solution that I’ve spent 2.5h on yesterday (I’ve implemented it as a route guard and it was almost perfect :))

filrak · 2019-02-09T18:18:44Z

@pkarw i was thinking more about having this mapping property in product object (so it can be searched through elasticsearch) not as one downlaodable api endpoint.

Regarding pattern matching - * will also work.

This is not a redirect if we keep it in the same route. Your problem is with browser history so it's worth investigating if the redirection record can be ommited since there is no actual redirect happening at all - only a javascript code adding route to browser history (for example resolving this route.name before the actual routing process happens).

I'm not tied to my solution if you tested it out and it didn't work.

Apart from this I think implementing logic from magento backend and forcing frontend devs to use different patterns they used to is opposite of what we are trying to achieve with Vue Storefront and it's backend decoupling approach. What benefits we have over magento if we will start complicating things like them?
It's hard to understand it even for me since it's a completely different way of thinking that I'm used to in frontend world (and it's a symptom of bigger problem bc we already have things like this in VS that are not intuitive to learn for frontend devs).

In other words - if we can avoid complexity and unknown patterns - we should try ;). Specifically there shouldn't be any Vuex store here since it's nothing related to managing data.

So assuming I understood you well mix of this two solutions would be just one component (instead of whole module) similar to what <router-view> is. Basically our router on top of the router.

<template>
  <component :is="pageType" />
</template>

<script>
export default {
  data () {
    return {
      pageType: null
    }
  },
  props: {
    uniqueId: { required: true } // unique identifier used to specify which page we should render
  },
  methods: {
    setPage () {
      const pageData = getPageData(identifierFromRoute) // gets Vuex data (which previously was route params) for a given page required to fetch it (like product SKU etc) and type of page to render
      setPageData(pageData) // sets Vuex for given product/category/whatever was matched by getPageData()
      this.pageType = pageData.type
   }
}
</script>

// Edited many times, read again if you already did from notification in email

pkarw · 2019-02-09T19:02:18Z

Ok! Let me work on this for a little bit longer so I would be able to show more clearly how I see it working. Yesterday I was more focused on checking the options than on final solution :) thanks for the feedback it’s very helpful 👍

…tponed

…s fixed

This static section very depends on the current database structure - so I removed it

pkarw · 2019-02-15T20:31:25Z

TODO:

caching support for the mappings
tests
error handling
refactoring (I'll try to merge UrlClientDispatcher and UrlServerDispatcher into one component)
overall tests with the config.seo.urlDispatcher=false
multistore tests

pkarw · 2019-02-15T20:32:21Z

core/modules/url/store/actions.ts

+    })
+  },
+  mapUrl ({ state, dispatch }, { url, query }) {
+    const parsedQuery = typeof query === 'string' ? parseURLQuery(query) : query


I'll change this one to https://www.npmjs.com/package/query-string

filrak · 2019-02-16T09:28:03Z

@pkarw can you describe more or less where this URL comes from and how on abstractive level this mechanism works? I have troubles understanding it.

Also i think instead of going into a simplified architecture that can be adopted with nuxt and vue-cli we are going the opposite with custom hydration process

filrak

This PR is extremely overcomplicated, violating separation of concerns, repeating logic, breaking native behavior that may lead into tons of potential future problems when we want to modularize VS. Every if check is a code smell and at this point logic is unreadable.

I suggest meeting and discussing the approach to handle this problem first and code it when we will have proper answers since right now it's impossible to review even the idea since it's not clear at all from the code.

Imho in this form it'll break few milestones of simplifying VS.

Also it contains many of the things @lukeromanowicz with me and @patzick identified as ones we need to got rid from VS to make it more udnerstandable and easier to maintain. Right now bc of things like this there is only one person in the core team that understands most of VS logic and why every new core team member tells su that we shoudl refactor it.

People are using VS bc there is no better option on the market yet and direct competition is magento so basically everything is simplier than this but it's gonna change as soon as competitors will become mature and it will turn out that it's impossible to scale and customize VS in easy way and it'll become second Magento.

Sorry if this sounds mean - it's not meant to. It's just a honest opinion on overcomplicating our logic and 'happy coding' process instead of debating about problems and figuring out proper way to solve them.

// edited multiple times, please read again when used email notifications ;)

filrak · 2019-02-16T09:36:12Z

core/modules/url/hooks/afterRegistration.ts

@@ -0,0 +1,5 @@
+import { Logger } from '@vue-storefront/core/lib/logger'


file is empty

filrak · 2019-02-16T09:36:20Z

core/modules/url/hooks/beforeRegistration.ts

@@ -0,0 +1,5 @@
+import { AsyncDataLoader } from '@vue-storefront/core/lib/async-data-loader'


file is empty

filrak · 2019-02-16T09:37:04Z

core/modules/url/index.ts

@@ -0,0 +1,79 @@
+import { module } from './store'


helpers should be placed in proper folder, not in a root.

filrak · 2019-02-16T09:38:14Z

core/modules/url/pages/UrlClientDispatcher.vue

+
+            if (from.name) { // don't execute the action on the Client's side of the SSR actions
+              if (_matchingComponentInstance.asyncData) {
+                AsyncDataLoader.push({


AsyncDataLoader will be removed in next version, instead update to vue 2.6 and use native nehavior

filrak · 2019-02-16T09:39:26Z

core/modules/url/pages/UrlClientDispatcher.vue

+export default {
+  computed: {
+    page: () => {
+      return _matchingComponentInstance


this shouldn't be a computer property + you shouldn't use arrow functions in computed properties

filrak · 2019-02-16T09:41:16Z

core/modules/url/types/UrlState.ts

+// This object should represent structure of your modules Vuex state
+// It's a good practice is to name this interface accordingly to the KET (for example mailchimpState)
+export interface UrlState {
+  dispatcherMap: {}


missing typings

pkarw · 2019-02-16T09:53:52Z

Let’s discuss this on Monday - core planing at 10:00.
Please bring some constructive counter proposals. This structure is not proposed by coincidence. It’s extremely hard to develop plugin solution for touting the irls both csr and ssr

filrak · 2019-02-16T09:54:56Z

Ok, but please just describe how it works. We can't understand it with @patzick

// btw i edited prev post to better ilustrate my point

pkarw · 2019-02-16T10:00:48Z

UrlDispatcher is checking the custom url with mappingFallback - method that can be customized/overridden. It’s returning the route data where this irks is pointing to. So this mechanism was created as a separate layer over existing Product and Categor /CMS pages. More over it’s supporting all content types not only lroducts and categories as url dispatchers operates on the touring tables defined in the theme. I had huge problems with dynamically loading and hydrating the components and with the changes of url within the same routes - for example clickingvrelated products on product page don’t execute full router flow and not resolving lazy loaded components (kind of cache).

pkarw · 2019-02-16T10:01:15Z

Overall I’ve spent 24 hours in total just on experiments.:;

filrak · 2019-02-16T10:02:31Z

where this custom url comes from?

pkarw · 2019-02-16T10:03:59Z

Alternative solution is to totally refactor / rewrite url dispatching - for example not rely on sky but only on url_path in product.js - marching “*” with both product and category pages and then executing kind of “next” if this url doesn’t match category but product etc. but this will be really hard to understand ...

Now it’s pretty simple: this is another layer - separate module - which is optional. Can be plug in if you like to have these routable urls.

filrak · 2019-02-16T10:06:02Z

core/build/webpack.base.config.ts

@@ -78,6 +78,7 @@ export default {
      inject: isProd == false
    })
  ],
+  devtool: 'source-map',


it's longest sourcemap alghorithm that imcreases build time
We prev changed it to increase build time speed, it should be available only under special flag imho and completely disabled in prod

Change it please to other one. Currently it’s disabled at all which makes the debugging almost impossible. It was a big oversaw issue with the latest release :(

It’s disabled in prod with my change. You haven’t checked the *.prod.ts config where it’s disabled.

Ok, sorry i overseen this

pkarw · 2019-02-16T10:10:21Z

core/modules/url/store/actions.ts

+   * Router mapping fallback - get the proper URL from API
+   * This method could be overriden in custom module to provide custom URL mapping logic
+   */
+  mappingFallback ({ commit, dispatch }, { url, params }) {


Here is the fallback if route hasn’t been cached we’re mapping url to route using server call - by default request for category/product are separated but it can be optimized with just one call to /api/mapUrl - or something like this. Here is the source for where the urls come from @filrak

Please analyze this solution carefully and propose other approach as really: there are not many different options for having this feature. At last from my r&d. I’m totally frustrated with this feature to be honest. Would love if somebody else will do this better way be cause I’m also not very happy with the overall complication and of the duplicates logic of client-entry.ts vs UrlClientDispatcher.ts.

I’ve had some partial successes with other approaches but always there was some deal breaker. The biggest problem was with not executing the full Vue-router flow in case when route has not changed. Imagine the situation when user request is handled by the by “urldisparcher” and user then clicks another time product link. Vue is just executing the watch on $route inside current component. In case if you are on product component clicking on category link you have no way to lazy load category component and replace the current one ....

pkarw · 2019-02-16T10:18:33Z

I’m open to other solutions / PoC @filrak and @patzick. Acceptance criteria were like this:

there is no distinction in the url to which content it points out: /something/category could point to other content than /somethingelse/product- so the urls are totally custom
we don’t want to do redirects: page is displayed under custom url with no additional url change after content type is discovered
ssr must work
offline mode must work
routes must be cached for not executing the additional network request if we could have obtained the url mapping from product/list, category/list before hand and cache it
the routing mechanism should be opened for other pages / content types added by the user (CMS pages under the same url structure / whatever)

filrak · 2019-02-16T10:28:17Z

Thanks we will try to figure it out. Basically we will just try to take your solution as a base and see how we can simplify it in terms of code. If it can't be then we just need a different solution. I believe that if achieving something requires too much conplication then the approach is wrong and it should be rethinked from a different perspective

pkarw · 2019-02-16T10:28:22Z

To not make the core more complicated we can probably publish this module as a separate / non-core one. Maybe someone will bring better solution later :)

pkarw · 2019-02-16T10:33:28Z

@filrak sure, that makes sense. I haven’t found any other dynamic routing solution for Vue. It could be cool hook for contribution if done properly.

The simplest way it should work would be if Vue-router supported plugins / or dynamic routes so we could inject them dynamically.

In that case we could have just insert static mapped url { path: “/something/product”, params: { sku: mappedSku.....

This approach would be the cleanest one. It must be dynamic and we can’t geberate static routing for all urls as there could be lol 100k+ entries on mid size fashion store.

Maybe there is way to insert routings dynamically so we don’t have to deal with all the other hacks like UrlClientDispatcher things.

Haven’t tested dynamic “addRoutes” executed in the guard.

Maybe we should inject addditiknal static route (from url/mapUrl) in the guard and call just next????

pkarw · 2019-02-16T10:36:28Z

core/modules/url/index.ts

+}
+
+export const UrlDispatcherGuard = (to, from, next) => { 
+  if (store.state.config.seo.useUrlDispatcher) {


Maybe we should just leave this guard for “*” and dynamically call “router..addRoutes([routeData]); next();?” to create dynamic route, with “to.fullPath” set as “path” and leave the all dirty job to Vue Router?

Can You test this way @filrak and @patzick as a PoC?

I can take care of this

filrak · 2019-02-16T10:38:54Z

Dynamic routes generation seems fine. I'll ask Eduardo who is a current maintainer of Vue router about this approach

pkarw · 2019-02-16T10:54:32Z

If this dynamic routes will work then we’re saved 😎it will be ultimate solution: simple, elegant and compatible with all the acceptance criteria.

Thanks for this feedback anyway 👍

pkarw · 2019-02-16T10:56:48Z

vuejs/vue-router#1083
vuejs/vue-router#888

The first link looks like our case; the second one looks like our new proposed solution :)

filrak · 2019-02-16T16:39:31Z

Awesome. So i think we have some common ground now! ❤️

filrak · 2019-02-16T16:48:23Z

Btw we should also think how to get rid of this config 'ifs' all around the project. Its producing module-specific side effects and makes our codebase bigger even when someone is not using particular features + it violates separwtion of concerns and encapsulation . I guess we need some kind of composition here instead of conditions

pkarw · 2019-02-17T10:52:04Z

Closing in favour of #2446

PoC - multipart, dynamic URL rewrite

4fb53e5

pkarw requested review from Igloczek, patzick and filrak February 8, 2019 23:08

This was referenced Feb 8, 2019

Improve the SEO support #1226

Closed

SEO friendly urls #2010

Closed

filrak requested changes Feb 9, 2019

View reviewed changes

pkarw added 14 commits February 12, 2019 07:11

Logger fixes + url mapping

a2f3405

Error handling on URL dispatcher

e499ac5

Product route links + error handling improvements

a365f4b

Further works on UrlDispatcher - including route params

baddb48

Update actions.ts

97d28a8

Further works on URL - Product + Category page refactor + hydrate pos…

ff0a6ab

…tponed

WIP - document.location as a workaround for same route redirects

648fd76

Source maps added, UrlClientDispatcher added

3d89013

preAsyncData action support added to UrlClientDispatcher + breadcrumb…

0d28943

…s fixed

Category links

f9f73c8

Departments menu removed from Footer

3885e6a

This static section very depends on the current database structure - so I removed it

product/list extended for registerMapping

426d37e

category/list extended with dispatcher mapping registration

8c359d2

Merge branch 'develop' into feature/2010_seo_friendly_urls

f328304

pkarw commented Feb 15, 2019

View reviewed changes

filrak requested changes Feb 16, 2019

View reviewed changes

filrak reviewed Feb 16, 2019

View reviewed changes

pkarw commented Feb 16, 2019

View reviewed changes

pkarw mentioned this pull request Feb 16, 2019

Feature/2010 seo friendly urls dynroutes #2446

Merged

5 tasks

pkarw closed this Feb 17, 2019

		@@ -0,0 +1,5 @@
		import { Logger } from '@vue-storefront/core/lib/logger'

		@@ -0,0 +1,5 @@
		import { AsyncDataLoader } from '@vue-storefront/core/lib/async-data-loader'

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

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

Conversation

pkarw commented Feb 8, 2019

Related issues

Short description and why it's useful

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

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

Upgrade Notes and Changelog

Contribution and currently important rules acceptance

pkarw commented Feb 8, 2019

pkarw commented Feb 8, 2019

pkarw commented Feb 8, 2019 • edited Loading

filrak commented Feb 9, 2019 • edited Loading

filrak left a comment

Choose a reason for hiding this comment

pkarw commented Feb 9, 2019

pkarw commented Feb 9, 2019

pkarw commented Feb 9, 2019

filrak commented Feb 9, 2019 • edited Loading

pkarw commented Feb 9, 2019

pkarw commented Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

filrak commented Feb 16, 2019 • edited Loading

filrak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkarw commented Feb 16, 2019

filrak commented Feb 16, 2019 • edited Loading

pkarw commented Feb 16, 2019

pkarw commented Feb 16, 2019

filrak commented Feb 16, 2019

pkarw commented Feb 16, 2019

filrak Feb 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkarw commented Feb 16, 2019

filrak commented Feb 16, 2019

pkarw commented Feb 16, 2019

pkarw commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filrak commented Feb 16, 2019

pkarw commented Feb 16, 2019

pkarw commented Feb 16, 2019 • edited Loading

filrak commented Feb 16, 2019

filrak commented Feb 16, 2019 • edited Loading

pkarw commented Feb 17, 2019

pkarw commented Feb 8, 2019 •

edited

Loading

filrak commented Feb 9, 2019 •

edited

Loading

filrak commented Feb 9, 2019 •

edited

Loading

pkarw commented Feb 15, 2019 •

edited

Loading

filrak commented Feb 16, 2019 •

edited

Loading

filrak left a comment •

edited

Loading

filrak commented Feb 16, 2019 •

edited

Loading

filrak Feb 16, 2019 •

edited

Loading

pkarw commented Feb 16, 2019 •

edited

Loading

filrak commented Feb 16, 2019 •

edited

Loading