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

Resolved : JS files located outside the web/js directory #16582

Conversation

@hitesh-wagento
Copy link
Contributor

commented Jul 6, 2018

Description

Some JS files are direct children of web rather than web/js. This does not follow instructions from the dev docs thus is confusing.

Fixed Issues (if relevant)

  1. #16302: JS files located outside the web/js directory

Expected result

  1. JS files to be inside web/js as per the dev docs.

Actual result

  1. JS files are direct children of web, this is inconsistent with the dev docs.

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

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

Hi @hitesh-wagento. 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 {$VERSION} instance - deploy vanilla Magento instance

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

@hitesh-wagento hitesh-wagento changed the title [Changed front js file locations] [Resolved : JS files located outside the web/js directory] Jul 6, 2018

@hitesh-wagento hitesh-wagento changed the title [Resolved : JS files located outside the web/js directory] Resolved : JS files located outside the web/js directory Jul 6, 2018

@VladimirZaets VladimirZaets self-assigned this Jul 6, 2018

@VladimirZaets

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

Hi @hitesh-wagento, due to Magento backward-compatible guide we can't move or rename files.
These changes are valid for 2.3 version, please create new one PR based on 2.3-develop branch.
Thanks

@ihor-sviziev

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Hi @hitesh-wagento,
As we discussed in #16708 (comment) - let's make this PR backward compatible and I will process it first.

@ihor-sviziev

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

Hi @hitesh-wagento,
We are having discussion about this specific case with @VladimirZaets, so please wait once we will finalize out general vision how it should be done. I will update you after discussion.

@hitesh-wagento

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Hi @ihor-sviziev , @VladimirZaets

I have completed changes as per your suggestion please review and let me know if any changes required.

Thanks

@VladimirZaets

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Hi, @hitesh-wagento thanks for fixes. Please resolve the branch conflicts. As I see you apply the fix that was provided by @ihor-sviziev

 map: {
         '*': {
            transparent: 'Magento_Payment/transparent'
            transparent: 'Magento_Payment/js/transparent',
            'Magento_Payment/transparent': 'Magento_Payment/js/transparent'
         }
     }

you should to do it for each cases.
As example, currently you have

map: {
         '*': {
            captcha: 'Magento_Captcha/captcha'
            captcha: 'Magento_Captcha/js/captcha'
         }
     }

also you should add `'Magento_Captcha/captcha': 'Magento_Captcha/js/captcha'

@hitesh-wagento

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Hi @ihor-sviziev

As per our Slack discussion I have completed changes please review and let me know if any changes required.

Thanks

@@ -123,7 +123,7 @@
<script type="text/x-magento-init">
{
"a[role='back']": {
"Magento_SendFriend/back-event": {}
"Magento_SendFriend/js/back-event": {}

This comment has been minimized.

Copy link
@ihor-sviziev

ihor-sviziev Jul 19, 2018

Contributor

We need to create mapping between old and new path in requirejs-config.js file in Magento_SendFriend module

This comment has been minimized.

Copy link
@hitesh-wagento

hitesh-wagento Jul 19, 2018

Author Contributor

Applied changes as per our slack conversion.

@ihor-sviziev ihor-sviziev self-assigned this Jul 19, 2018

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Hi @ihor-sviziev, thank you for the review.
ENGCOM-2415 has been created to process this Pull Request

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Hi @ihor-sviziev, thank you for the review.
ENGCOM-2415 has been created to process this Pull Request

@hitesh-wagento

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

Hi @ihor-sviziev , @VladimirZaets

Any update in this PR ?

Thanks

@ihor-sviziev

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

Hi @hitesh-wagento,
This PR didn't passed testing yet. That's why it didn't merged yet.

@magento-engcom-team magento-engcom-team merged commit 2301450 into magento:2.2-develop Aug 7, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
magento-engcom-team pushed a commit that referenced this pull request Aug 7, 2018
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Hi @hitesh-wagento. Thank you for your contribution.
We will aim to release these changes as part of 2.2.7.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

@magento-engcom-team magento-engcom-team added this to Ready to Port in Ports Wanted Aug 7, 2018

@hitesh-wagento

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Hi @ihor-sviziev

Do I need to create ports for this PR ?
Because I have already created PR for this : #16708

Thanks

@ihor-sviziev

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Hi @hitesh-wagento,
No, you should not

@magento-engcom-team magento-engcom-team moved this from Ready to Port to Port to 2.3 Created in Ports Wanted Aug 7, 2018

@magento-engcom-team magento-engcom-team moved this from Port to 2.3 Created to Port to 2.3 Merged in Ports Wanted Sep 12, 2018

@keharper

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

@hitesh-wagento You linked to devdocs at the top of this PR. It looks like you changed the code to meet the standards set in the documentation, but I want to verify whether you made any documentable changes.

@hitesh-wagento

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

@hitesh-wagento You linked to devdocs at the top of this PR. It looks like you changed the code to meet the standards set in the documentation, but I want to verify whether you made any documentable changes.

Hi @keharper

I have changed as per suggested by ihor-sviziev

@rbayet

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

Hi @ihor-sviziev, @hitesh-wagento,

I'm not sure it's the best of places to address that, but I didn't want to re-open #16302 by adding a comment to it.

My issue is it seems the changes seem to be BC under some conditions.

For instance, if in a 2.2.5 :

  • I manually relocate Magento_Search/form-mini to Magento_Search/js/form-mini
  • I apply manually the changes in Magento_Search/requirejs-config.js
var config = {
    map: {
        '*': {
            quickSearch: 'Magento_Search/js/form-mini',
            'Magento_Search/form-mini': 'Magento_Search/js/form-mini'
        }
    }
};
  • I have a third party module/theme overriding the "quickSearch" requirement and referencing the native/legacy path as "mageQuickSearch"
var config = {
    map: {
        '*': {
            mageQuickSearch : 'Magento_Search/form-mini',
            quickSearch: 'Third_PartyModule/js/form-mini'
        }
    }
};

'mageQuickSearch' being there to be referenced as a define() dependency in 'Third_PartyModule/js/form-mini'.

Problem: The path mapping is not taken into account, either in blank or luma, in developer or production modes : a 404 error is thrown for ".../Magento_Search/form-mini.js".

IF I remove "mageQuickSearch" from the third party requirejs-config.js and replace it in the 'Third_PartyModule/js/form-mini' define() by the native/legacy path 'Magento_Search/form-mini', then it works as expected.

Am I missing something or does that mean there is still a risk of BC break across the board ?

Regards

rbayet added a commit to rbayet/elasticsuite that referenced this pull request Nov 19, 2018
Fixes Smile-SA#1186 - support web/js files relocation
To be introduced in 2.3.x and 2.2.7
With expected BC break in 2.2.x because magento/magento2#16582 does not seem to cover this particular case
See issue of origin magento/magento2#16302
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.