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

JS files relocation inside the web/js directory not BC in 2.3.x #19291

Closed
rbayet opened this issue Nov 21, 2018 · 9 comments
Closed

JS files relocation inside the web/js directory not BC in 2.3.x #19291

rbayet opened this issue Nov 21, 2018 · 9 comments
Labels
Component: Theme Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@rbayet
Copy link
Contributor

rbayet commented Nov 21, 2018

Summary

The issue #16302 has been fixed both in 2.2.x and 2.3.x lines but only the fix in 2.2.x is BC compatible by providing and "old_path : new_path" mapping in the requirejs-config.js.

Examples

For instance for the 2.2.x line for app/code/Magento/Authorizenet/view/frontend/requirejs-config.js
the combination of 8653ba1 and e0b09fd leads to the following changes :
From

var config = {
    map: {
        '*': {
            transparent: 'Magento_Payment/transparent'
        }
    }
};

To

var config = {
    map: {
        '*': {
            transparent: 'Magento_Payment/js/transparent',
            'Magento_Payment/transparent': 'Magento_Payment/js/transparent'
        }
    }
};

(See https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Authorizenet/view/frontend/requirejs-config.js)

This means any reference to 'Magento_Payment/transparent' in a third party extension's JS component define() dependency will point correctly to 'Magento_Payment/js/transparent'.

On the other hand, in the 2.3.x line, the changes are simply (see 0ce439b)
From

var config = {
    map: {
        '*': {
            transparent: 'Magento_Payment/transparent'
        }
    }
};

To

var config = {
    map: {
        '*': {
            transparent: 'Magento_Payment/js/transparent'
        }
    }
};

(See https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Authorizenet/view/frontend/requirejs-config.js)

This time, any third party extension's JS component define() dependency to 'Magento_Payment/transparent' will fail and break the extension.

Proposed solution

As the BC solution found for the 2.2.x line is not that intrusive, it could also be adopted for the 2.3.x line, eliminating (in some conditions) the need to address those JS files relocation for third party extension authors.

Regards,
Richard.

@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Nov 21, 2018

Hi @rbayet. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me $VERSION instance

where $VERSION is version tags (starting from 2.2.0+) or develop branches (for example: 2.3-develop).
For more details, please, review the Magento Contributor Assistant documentation.

@rbayet do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Nov 21, 2018
@rbayet
Copy link
Contributor Author

rbayet commented Nov 22, 2018

Sorry, can't edit the previous comment to tick the checkboxes when appropriate.

The issue is purely related on BC for third party extensions (modules, themes), so it cannot be reproduced on a vanilla Magento instance alone.

I could indicate steps to reproduce, but it will require the installation of a third party extension.

@romainruaud
Copy link
Contributor

Hello @magento-engcom-team

Just to be clear, this one is a "Developer experience issue", making us unable to provide proper compatibility with Magento 2.2 AND 2.3 on our Elasticsuite extension.

@rbayet already explained well what we are facing here, and I'm sure we are not alone to build JS components based on the JS components that were affected by this decision to move them.

On our specific case, it's due to the 'Magento_Search/form-mini' that has been moved, but since many other files are concerned, I'm wondering why you decided to not keep the BC version in 2.3 (you introduced it and used it for 2.2.x versions).

Question is : did you squeeze it on purpose (no BC for major version), or is it a mistake ?

We need to know it in order to design our own compatibility (and BC) map for the next version of our extension which is meant to support Magento 2.3.

Regards

@romainruaud
Copy link
Contributor

And btw, @rbayet and me would be glad to submit a PR to fix this one if you ask us to.

Regards

@romainruaud
Copy link
Contributor

Just to be transparent : the comment I removed was the auto response when assigned. I let you manage the lifecycle/assignment of this issue by yourself.

Regards

@romainruaud romainruaud added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Nov 23, 2018
@magento-engcom-team magento-engcom-team removed the Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed label Nov 23, 2018
@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Nov 23, 2018

@romainruaud Thank you for verifying the issue.

Unfortunately, not enough information was provided to created internal ticket. Please consider adding the following:

  • Add "Component: " label(s) to this ticket based on verification result. If uncertain, you may follow the best guess
  • Add "Reproduced on " label(s) to this ticket based on verification result

Once all required information is added, please add label "Issue: Confirmed" again.
Thanks!

@romainruaud romainruaud added Component: Theme Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Nov 23, 2018
@magento-engcom-team
Copy link
Contributor

@romainruaud Thank you for verifying the issue. Based on the provided information internal tickets MAGETWO-96618 were created

@magento-engcom-team magento-engcom-team added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Nov 23, 2018
@DrewML
Copy link

DrewML commented Nov 30, 2018

Thanks for the report @rbayet! I have to assume the missing BC path in 2.3.0 was a mistake. Would be happy to accept a PR from you or @romainruaud with the fix.

@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Dec 10, 2018
@magento-engcom-team
Copy link
Contributor

Hi @rbayet. Thank you for your report.
The issue has been fixed in #19583 by @rbayet in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Theme Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests

4 participants