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

Invalid shims in Magento core break `core-bundle.js` at runtime #6

Open
EirikSl opened this issue Aug 22, 2019 · 13 comments
Open

Invalid shims in Magento core break `core-bundle.js` at runtime #6

EirikSl opened this issue Aug 22, 2019 · 13 comments

Comments

@EirikSl
Copy link

@EirikSl EirikSl commented Aug 22, 2019

Screenshot 2019-08-22 12 17 44
That is a list of loaded JS files in waterfall order.

Pay attention to the order of files core-bundle.js and jquery.cookie.js
core-bundle.js includes jquery-storageapi.js at the same time, storageapi is depended from
'jquery/jquery-storageapi': {'deps': ['jquery/jquery.cookie']}

As you see from Image jquery.cookie.js is loaded later bundle. In result, I have some Js error because of undefined Jquery methods.

Looks like all files included in the bundle should be checked for dependencies, and I afraid they should be sorted in the bundle in the correct order.

@DrewML

This comment has been minimized.

Copy link
Member

@DrewML DrewML commented Aug 22, 2019

Thanks for the report Victor.

It looks like there are a few things going on here.

Issue with jquery.cookie

In Magento core, the following shim config is present (as you mentioned):

'jquery/jquery-storageapi': {
   'deps': ['jquery/jquery.cookie']
}

But, jquery/jquery-storageapi is already an AMD module - shims are meant for non-AMD modules. From the RequireJS docs:

image

So this is a bug we need to fix in Magento core - @adifucan is working on getting this fix landed in core. For the time being, you have a few options:

  1. Edit jquery/jquery-storageapi and manually add jquery/jquery.cookie to the dependency list
  2. Map jquery/jquery-storageapi to a different file, where you first import jquery/jquery.cookie, then jquery/jquery-storageapi
  3. Add a new requirejs-config.js in your theme that is just:
var config = {
   deps: ['jquery/jquery.cookie']
};

Loading of requirejs-config.js

I noticed that you're currently loading core-bundle.js and requirejs-config.js. This was a good guess since we don't have Magento_Baler done yet, but you'll need to make some small changes here.

When baler runs, it adds requirejs-bundle-config.js to each published theme root. You should be loading this instead of requirejs-config.js. This file is injected with configuration that tells RequireJS which modules will be in core-bundle.js, which allows core-bundle.js to be loaded asynchronously without worrying about races.

image

Besides that, you don't need a script tag for core-bundle.js - RequireJS will automatically fetch it. Instead, change your script tag to a preload to hint to the browser to start the download ASAP.

Other

Looks like all files included in the bundle should be checked for dependencies, and I afraid they should be sorted in the bundle in the correct order.

This is actually the way things work! All dependencies are traced in traceAMDDependencies.ts.

Then, in computeDepsForBundle.ts, we crawl the dependency graph depth-first (the same order RequireJS executes modules at runtime), and build the final module list, in order.

@EirikSl

This comment has been minimized.

Copy link
Author

@EirikSl EirikSl commented Aug 23, 2019

Thank you for such a detailed reply and sorry for not clear situation with "requirejs-config.js" - this is renamed requirejs-bundle-config.js in my project

We have shim for not only jquery/jquery-storageapi" but for "mage/common" too. I think you've already seen this in baller log.
Found shim configuration for "mage/common"...
Found shim configuration for "jquery/jquery-storageapi"....

And as you understand the wrong shim can be in requirejs-config.js from any other third-party Magento extensions and we can't override requirejs-config.js for separate extension in our child theme. So maybe fix #3 is the best for such situations.

There are 2 questions:
1 Can baler automate such fix
2 If not for #1. Will core fixes from @adifucan come to Magento 2.2?

@DrewML

This comment has been minimized.

Copy link
Member

@DrewML DrewML commented Aug 26, 2019

Can baler automate such fix

I think it's possible for us to auto-fix in certain situations, but I'm not quite convinced that would be the right approach. Because it's not a supported way to use the RequireJS API, we'd be building in a hack that could have unintended side-effects. Need to think on this a bit more.

Will core fixes from @adifucan come to Magento 2.2

Backports will happen for all currently supported versions of Magento. I believe there will be composer patches (or something similar) released in the interim for those that can't wait for a new release.

@friendscottn

This comment has been minimized.

Copy link

@friendscottn friendscottn commented Aug 30, 2019

Baler, gives me two additional warnings about shims besides the jquery/jquery-storageapi shim.

Found shim configuration for "mage/common", but it is already an AMD module. RequireJS does not support shimming AMD modules. You may see unexpected behavior as a result.
Found shim configuration for "moment", but it is already an AMD module. RequireJS does not support shimming AMD modules. You may see unexpected behavior as a result.

They seem to stem from the same core file as the issue above.
vendor/magento/module-theme/view/base/requirejs-config.js

var config = {
   ...
    'shim': {
        ...
        'mage/common': ['jquery'],
        ...
        'moment': {
            'exports': 'moment'
        },
        ...
        'jquery/jquery-storageapi': {
            'deps': ['jquery/jquery.cookie']
        }
    }
   ...
}

I'm running against 2.2.9, but I can see the same shims in 2.3.develop too.
Are these two shims also on @adifucan 's radar to fix?

@adifucan

This comment has been minimized.

Copy link

@adifucan adifucan commented Sep 1, 2019

Hi @friendscottn! Thanks for reporting. Those 2 shims are also eliminated in my fix. Next week it will be merged to mainline. I’ll keep you updated. Thanks!

@friendscottn

This comment has been minimized.

Copy link

@friendscottn friendscottn commented Sep 1, 2019

Hey that's great @adifucan. Thank you!

@DrewML DrewML changed the title Error in internal core-bundle dependencies Invalid shims in Magento core break `core-bundle.js` at runtime Sep 3, 2019
@DrewML

This comment has been minimized.

Copy link
Member

@DrewML DrewML commented Sep 3, 2019

Added a doc on this that we now surface in the CLI: https://github.com/DrewML/baler/blob/master/docs/INVALID_SHIM.md

┌─[andrewlevine]  [~/sites/baler]
└─▪ baler
Collecting data about themes and modules from the store
Data collection completed in 145ms
Found 1 eligible theme to optimize
  - Magento/luma
Starting to bundle core-bundle for Magento/luma with 95 dependencies
One or more invalid shim configurations were found while bundling Magento/luma. See https://github.com/DrewML/baler/blob/master/docs/INVALID_SHIM.md
   - mage/common
   - moment
   - jquery/jquery-storageapi
@tmotyl

This comment has been minimized.

Copy link

@tmotyl tmotyl commented Sep 3, 2019

@adifucan can you crosslink here PR/issue on Magento side where the shim are goong to be fixed?

@adifucan

This comment has been minimized.

Copy link

@adifucan adifucan commented Sep 3, 2019

@tmotyl it's merged today into mainline within this commit: magento/magento2@db43c11

@Vinai

This comment has been minimized.

Copy link

@Vinai Vinai commented Oct 6, 2019

@adifucan Will the PR make it into the 2.3.3 release?

@adifucan

This comment has been minimized.

Copy link

@adifucan adifucan commented Oct 7, 2019

@Vinai unfortunately not. but it will be available in 2.3.4

@Vinai

This comment has been minimized.

Copy link

@Vinai Vinai commented Oct 7, 2019

@adifucan Thank you for the reply, even though that’s sad news.
I think I‘ll try to put a vendor based patch for 2.3.2 and 2.3.3 up somewhere.

@tdgroot

This comment has been minimized.

Copy link
Member

@tdgroot tdgroot commented Oct 17, 2019

I created a patch gist for the jquery.cookie problem: https://gist.github.com/tdgroot/f95c398c565d9bbb83e0a650cdf67617

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