magento / m2-devtools

Helpful in-browser debugging/inspection tools for the Magento 2 Front-End
Open Software License 3.0
172 stars 48 forks source link

Tracking of customer account pages not working #45

Open DrewML opened 5 years ago

DrewML commented 5 years ago

This issue is a:

Environment

Question Answer
Magento version 2.3.0
Browser + version N/A
node.js version (node -v) N/A
npm version (npm -v) N/A

Description

When clicking around in "Record" mode for the RequireJS Bundle Config generator, results are not captured for customer account pages (reported by @davidverholen).

This is very likely the result of a heuristic that is not 100% correct, and we do not surface that error anywhere in the UI.

Expected result:

The fix for this should be to extend _getPageConfigType in interop.ts to have a more reliable heuristic.

Note: A hard requirement of fixing this will be that it does not require installing any additional modules. One of the goals of the Bundle Generator is to be able to generate config with 0 changes to the Magento store.

Possible solutions:

We can add in some additional heuristics for pages that don't match the pattern being looked for in _getPageConfigType. An example would be a hardcoded list of pages that we can't find the layout handle for, and some additional checks to figure them out.

davidverholen commented 5 years ago

it would be possible of course to create something reliable in the magento module that the extension could use.

But this would mean that you then need the magento module installed to generate the bundles, which currently is not required

DrewML commented 5 years ago

But this would mean that you then need the magento module installed to generate the bundles, which currently is not required

Yeah, I'm trying to avoid that as hard as possible. I want bundling to be as simple as possible, and being able to generate a config on a base install with no extra modules is appealing.

I'm working on a fix for this locally. Likely will take some iterations to get the heuristics right

DrewML commented 5 years ago

Took a bit of a whack at this, and it looks as if we'll have to change how the extension and m2 module work together to get this right.

Problem Magento_BundleConfig determines if there is a page-specific JS file to add to the page, based on whether a JS file exists in pub/static/bundles. As an example, if $fullActionName is downloadable_customer_products, the module will append pub/static/bundles/download_customer_products.js only if it exists.

There is a problem here: if we use the class name on the body to infer which handle should be updated, we won't be bundling as efficiently per area of the store.

Example When visiting the customer account section of the storefront, the layout handle rendered as a class on the body is customer-account-index. Similarly, on the account edit page, the layout handle rendered on the body is customer-account-edit.

This isn't great, because ideally we'd like a single bundle for all customer sections. In other words, we want our bundle to be associated with the customer_account handle, as that's used in most customer account page types. customer_account is an arbitrary handle, though, so it's not easy to infer that information.

Proposed Solution I think it's acceptable for default page types to be hardcoded into the extension. In other words, the extension would have a mapping that looks roughly like this:

const mappings = {
   'customer-account-index': 'customer_account',
   'customer-account-edit': 'customer_account'
};

We would then make 2 changes:

  1. The extension will generate a configuration file (along with build.js) for Magento_BundleConfig to read during a request (XML or JSON, does not matter).
  2. Magento_BundleConfig will be changed to read this configuration file during a request, and will include any bundles from the config that have a handle matching the handles in $context->getLayout()->getUpdate()->getHandles().
davidverholen commented 5 years ago

one counter example regarding the needed bundles of a page (during bundle config generation): The ce customer account mostly is not rendered using ui components so also mostly needs the shared bundle.

In contrary, in ee, for example, the requisition lists or the company account acls have it's completely own set of js components, which then would also be included into the shared or customer_account bundle.

So if you would bundle all js components for the customer account into one bundle, there would still be a pretty large overhead for something like customer_account_dashboard.

The same of course could happen for custom pages to an uncontrollable extend

davidverholen commented 5 years ago

really not sure about it / would need a bit of testing / investigation: Would it maybe make sense, to create the bundle names based on a combination of loaded layout handles / body classes? E.g. md5 hash of the body classes and then load them based on the applied layout handles.

Based on the premise that every layout handle can potentially add js components.

This could also catch edge cases where layout handles are added dynamically to a page based on specific circumstances, like showing different content for different customer groups for example.

The downside of this is probably less browser caching since most pages would have it's own bundle. Not sure which one would have the bigger impact here

davidverholen commented 5 years ago

just maybe (to continue speaking with myself here ;) ) build something like @DrewML s solution as a default, but add configurations to the browser extension to enable the user to customize the bundle configuration.

For example bundling all customer-account pages into one like @DrewML suggested. But adding a checkbox to divide it into multiple bundles with the combined layout handle approach.

The magento extension could then have a fallback mechanism. Since we hopefully all have the full page cache enabled in production, it should not really have a noticeable impact on performance testing for the different bundles.

You could then even extend it, building bundles for example for all pages which layout handles mostly start with customer. Extend it (by configuration) to build bundles for layout handles starting (mostly) with customer_xyz. Next step would be customer_xyc_abc and finally a combination of all layout handles (as a hash) (But this idea is extremely experimental right now :P )

DrewML commented 5 years ago

(Haven't forgotten about this, just bogged down with some other work atm. Will get back into this one soon)

davidverholen commented 5 years ago

As another point pro many small bundles: overall performance of a page will probably be better with larger reusable bundles by leveraging browser caching. From a SEO perspective, since google page speed insights (as far as I know) always only evaluates one page at a time, the smallest possible bundle size would be optimal for that (so no shared bundle at all as the absolute extreme)

DrewML commented 5 years ago

I'm working on a prototype that may solve this problem in a bit of a different way, but would yield more control over to developers. Will share when I have more details