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

Upgrade to Promise-based requirejs #19517

Closed

Conversation

kirmorozov
Copy link
Member

Description (*)

Changes introduce major performance upgrade for Front-end, removes extra computation and timeouts overhead.
These changes are upgrading requirejs internals with requirejs/alameda - Promise-based implementation.
*mostly compatible with original RequireJS.
In this PR incompatible changes are resolved.

Fixed Issues (if relevant)

  1. magento/magento2#19470: Debugging of JS issues is painful, upgrade to Promise-based requirejs
  2. ...

Manual testing scenarios (*)

Changes must be at low level and compatible with most modules.

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 on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @kirmorozov. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@sivaschenko
Copy link
Member

Hi @kirmorozov , thanks for your contribution! For such fundamental changes an architectural approval is required. Can please create a pull request to https://github.com/magento/architecture describing your proposition.

@kirmorozov
Copy link
Member Author

Hi, fellow,
Nothing major.
This is simple replacement, same require(), define() API and polyfill for Promise.
Browser support stays the same, for most APIs.
Plus it is easier to expose dependency tree between modules.

It does not change architecture outside requirejs, and requirejs-related internals.
only 's' property require is going away,
and adding special exposure of queue so mixin.js can mess with it.
Change is so deep, very few engineers will ever get there.

I'll handle issues with tests as soon as reproduce them locally.

@sivaschenko
Copy link
Member

Hi @kirmorozov , I mean you are actually updating the library we are relying on lib/web/requirejs/require.js. Such changes should get common attention and approval. That's why I had to ask you to communicate that to https://github.com/magento/architecture

@kirmorozov
Copy link
Member Author

jsbuild is doomed.
Please lets move on with removal of jsbuild-based magento bundles as they are inefficient.
Gulp or Grunt bundling provide uglyfied bundles along with source maps.

@DrewML
Copy link

DrewML commented Dec 11, 2018

We'll discuss more in the meeting (magento/architecture#47), but regarding the perf comment:

Changes introduce major performance upgrade for Front-end, removes extra computation and timeouts overhead.

Do you have before/after traces you can share? Would be nice to see where the time was being spent.

@sivaschenko
Copy link
Member

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, here is your new Magento instance.
Admin access: https://pr-19517.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@kirmorozov
Copy link
Member Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @kirmorozov. Thank you for your request. I'm working on Magento instance for you

@sivaschenko
Copy link
Member

Hi @kirmorozov can you please provide benchmark for this pull request as was requested on the architecture discussion meeting

@sidolov
Copy link
Contributor

sidolov commented Mar 7, 2019

Hi @kirmorozov , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Mar 7, 2019
@ghost
Copy link

ghost commented Mar 7, 2019

Hi @kirmorozov, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment