Refactor JavaScript mixins module

[krzksz]

Description (*)

This refactor started as an attempt to fix the fact that mixins were not loaded and applied for bundled modules when using advanced bundling technique. During the process I decided to dive a bit deeper and see if it is possible to improve the way Magento extends RequireJS functionality.

Bundling bug

First of all, after a lot of investigation I found that the only way for the plugin to retrieve true modules' paths no matter if they are bundled or not, is to prepare a separate context which excludes any bundles configuration.

Leaking of arguments

In the previous implementation, functions like applyMixins, load and overwritten define were directly passing down special arguments object, which is considered a bad practice as it may leads to performance degradation and memory leaks so I refactored it away.

Usage of require.eval

Overwritten define method used require.exec which calls unsafe and poor-performing eval to be used.

I understand why this approach was used, the problem is that original RequireJS's define is able to append additional dependencies by analysing module's source code which happens here for example for bootstrap.js.

Indeed, pushing the transformed item to a private globalDefQueue doesn't leave much hope for intercepting it in any other way but then I had a breakthrough. Instead of processing dependency names when they are added to the queue, we can do the same when they get transferred from globalDefQueue to default context's defQueue (which we have direct access to) and shifted from it by wrapping the shift method. All of this happens before any of the dependencies are actually loaded so thre is no performance degradation or delay in requests involved. No more eval, another win.

Wrapping global require and define

Solving the above point made it possible for me to actually get rid of overwriting global define function. Additionally, by wrapping default context's require method, which always gets the parameters in normalized order and shape I was able to remove a lot of code that recreated original logic in order to retain compatibility with all RequireJS features.

Fixed Issues (if relevant)

1. Mixins are not applied for advanced bundled modules #25586: Mixins are not applied for advanced bundled modules
2. Mixins are not applied for bundled modules baler#23: Mixins are not applied for advanced bundled modules

Manual testing scenarios (*)

1. Apply this change.
2. Verify that modules are still properly loaded and executed.
3. Verify that mixins are properly loaded and applied no matter if advanced bundling is used or not.

Questions or comments

To be honest, I hoped that it would be possible to completely remove the need for plugin itself somehow while retaining it's functionality but I haven't figure out the way yet. That said, I'm very happy with the results of a few weeks worth of evenings and digging into RequireJS internals and the most important goal was achieved.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @krzksz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[DrewML]

Exciting stuff! And getting rid of eval there gets us one step closer to eventually being able to implement a content security policy effectively.

Traveling at the moment so sending this over to our performance team to give a first round of reviews.

Thanks for the hard work on this!

[VladimirZaets]

@magento run all tests

[krzksz]

We are all green 🥳 However, now that I figured out how to write and run frontend unit tests locally, maybe it would be worth it to add some for mixins module?

[DrewML]

now that I figured out how to write and run frontend unit tests locally, maybe it would be worth it to add some for mixins module?

Some smallunit tests would definitely be nice, but I'd hope we already have some e2e tests covering us so I wouldn't say it's critical.

[krzksz]

All of my work here is done I think, please post your feedback if any additional changes are needed.

[krzksz]

@DrewML I actually added some unit tests, so we are on the safe side 🤗

[ghost]

@omiroshnichenko unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

[m2-assistant]

Hi @krzksz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[ashar01]

Hi @krzksz , is this fix applied in magento 2.3.4 version? in magento 2.3.4 release notes i cant find it.

[adifucan]

Hi @krzksz, will you backport this fix to 2.3-develop? Or I can do it if you do not mind.

[krzksz]

I have no idea how to even do it so you have the green light 😁

[adifucan]

@krzksz Sorry, backport means just creating a PR with your fix to 2.3-develop 😁
You can do it on your own. Or I'll create a PR to 2.3-develop if you do not mind.

[krzksz]

I know, the issue is there is no 2.3-develop anymore. I'll ask other maintainers how to proceed here.

[krzksz]

I contacted Magento team and they are already working on a backport to 2.3 🤗

[adifucan]

Yeah, I talked to them. I cherry picked your commits to 2.3-develop. I tested it with advanced bundling and with baler js bundling either.
Modules and loaded and mixins are loaded as well.

[gwharton]

Running this PR against 2.3.4 I get javascript errors on page load. Anyone else seen these?

[krzksz]

@gwharton Can you add a conditional breakpoint for !names and see which module is affected? We are using this as a patch in multiple Magento 2.3.x versions with no issues so far.

[gwharton]

Yes, turns out to be this module. https://github.com/swissup/module-stickyfill

It is when this file is processed. view/frontend/web/js/stickyfill.js

In function defContext.defQueue.shift, queueItem[1] is null. It is null in the GlobalDefaultQueue. The old version seemed to handle this ok.

[krzksz]

I verified that it is indeed an issue with modules defined in CommonJS format. I'll prepare PR that addresses this regression shortly.

[krzksz]

Hey @gwharton sorry for confusion but I have debugged the issue thoroughly and it is indeed the module's issue. The problem is that it defines itself using

define(Stickyfill);

and Stickyfill is an object, while according to RequireJS's documentation it should be a function. I'm going to prepare an improvement to handle this scenario anyway, but I don't think it is a blocker anymore.

[micwallace]

Hey @magento-engcom-team

Is there any reason why this patch hasn't been included in the 2.3.4 or 2.3.5-p1 releases?