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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added JS tests for yii.js (#12840) #13316

Merged
merged 11 commits into from Jan 25, 2017

Conversation

Projects
None yet
4 participants
@arogachev
Member

arogachev commented Jan 2, 2017

Q A
Is bugfix? yes, it contains some bug fixes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues #13300, #13307, #13310, #13312
Partially fixed issues #13299 (only renamed the private function, renaming according public property is a BC break).
Other tested issues / PRs #8014, #11921, #10974, #11494, #10358, #10097 and others.

This one was not easy. 馃槹 Anyway I'm glad to present full test suite for yii.js. As with previous JS files, I performed deep code analysis, have looked through previous issues and history of changes, found some new issues and fixed some of them during writing tests.

Some of the code is pretty hard to test, but I found workarounds.

I will comment "in place" of made changes in yii.js, it will generate a bigger amount of notifications but I think it's more visual and intuitive for understanding.

Note: I added test for #8014, but this issue is not reproducible in jsdom. I created according issue there - jsdom/jsdom#1688.

* var pub = {
* // whether this module is currently active. If false, init() will not be called for this module
* // it will also not be called for all its child modules. If this property is undefined, it means true.
* isActive: true,
* init: function() {
* // ... module initialization code go here ...
* // ... module initialization code goes here ...

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Singular form, it goes.

@arogachev

arogachev Jan 2, 2017

Member

Singular form, it goes.

@@ -46,17 +46,18 @@ window.yii = (function ($) {
/**
* List of JS or CSS URLs that can be loaded multiple times via AJAX requests.
* Each item may be represented as either an absolute URL or a relative one.
* Each item may contain a wildcart matching character `*`, that means one or more
* Each item may contain a wildcard matching character `*`, that means one or more

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Typo.

@arogachev

This comment has been minimized.

@cebe

cebe Jan 2, 2017

Member

no need to comment on typo changes, it's obvious :)

@cebe

cebe Jan 2, 2017

Member

no need to comment on typo changes, it's obvious :)

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

@cebe Sorry, indeed. I'll keep this in mind.

@arogachev

arogachev Jan 2, 2017

Member

@cebe Sorry, indeed. I'll keep this in mind.

* any characters on the position. For example:
* - `/css/*.js` will match any file ending with `.js` in the `css` directory of the current web site
* - `/css/*.css` will match any file ending with `.css` in the `css` directory of the current web site

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Having js files in css folder is kind of weird. If it was written consciously - it's not a good example, otherwise - it's just a typo.

@arogachev

arogachev Jan 2, 2017

Member

Having js files in css folder is kind of weird. If it was written consciously - it's not a good example, otherwise - it's just a typo.

* - `http*://cdn.example.com/*` will match any files on domain `cdn.example.com`, loaded with HTTP or HTTPS
* - `/js/myCustomScript.js?realm=*` will match file `/js/myCustomScript.js` with defined `realm` parameter
*/
reloadableScripts: [],
/**
* The selector for clickable elements that need to support confirmation and form submission.
*/
clickableSelector: 'a, button, input[type="submit"], input[type="button"], input[type="reset"], input[type="image"]',
clickableSelector: 'a, button, input[type="submit"], input[type="button"], input[type="reset"], ' +

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Maximum length of 120 chars per line exceeded.

@arogachev

arogachev Jan 2, 2017

Member

Maximum length of 120 chars per line exceeded.

@@ -107,7 +108,7 @@ window.yii = (function ($) {
* @param cancel a callback to be called when the user cancels the confirmation
*/
confirm: function (message, ok, cancel) {
if (confirm(message)) {
if (window.confirm(message)) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

For easier creation of a stub for confirm in Sinon.

@arogachev

arogachev Jan 2, 2017

Member

For easier creation of a stub for confirm in Sinon.

@@ -143,74 +144,70 @@ window.yii = (function ($) {
* 'name2' => 'value2',
* ],
* ],
* ];
* ]);

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Missing closing parenthesis in JSDoc.

@arogachev

arogachev Jan 2, 2017

Member

Missing closing parenthesis in JSDoc.

* ```
*
* @param $e the jQuery representation of the element
* @param event Related event

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Missing parameter in JSDoc.

@arogachev

arogachev Jan 2, 2017

Member

Missing parameter in JSDoc.

*/
handleAction: function ($e, event) {
var $form = $e.attr('data-form') ? $('#' + $e.attr('data-form')) : $e.closest('form'),
method = !$e.data('method') && $form ? $form.attr('method') : $e.data('method'),
action = $e.attr('href'),
isValidAction = action && action !== '#',

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Was copy pasted. Converted check to strict.

@arogachev

arogachev Jan 2, 2017

Member

Was copy pasted. Converted check to strict.

pjaxPushRedirect = $e.data('pjax-push-redirect'),
pjaxReplaceRedirect = $e.data('pjax-replace-redirect'),
pjaxSkipOuterContainers = $e.data('pjax-skip-outer-containers'),
areValidParams = params && $.isPlainObject(params),

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Was copy pasted.

@arogachev

arogachev Jan 2, 2017

Member

Was copy pasted.

params = $e.data('params'),
pjax = $e.data('pjax') || 0,
usePjax = pjax !== 0 && $.support.pjax,
pjaxPushState = !!$e.data('pjax-push-state'),

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

All these variables are actually redundant. No additional manipulations are done with them, so we can do assignment in pjaxOptions.

@arogachev

arogachev Jan 2, 2017

Member

All these variables are actually redundant. No additional manipulations are done with them, so we can do assignment in pjaxOptions.

pjaxSkipOuterContainers = $e.data('pjax-skip-outer-containers'),
areValidParams = params && $.isPlainObject(params),
pjax = $e.data('pjax'),
usePjax = pjax !== undefined && pjax !== 0 && $.support.pjax,

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Fix for #13300.

@arogachev
pjaxContainer,
pjaxOptions = {};
if (usePjax) {
if ($e.data('pjax-container')) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Can be shortened for better readability.

@arogachev

arogachev Jan 2, 2017

Member

Can be shortened for better readability.

}
if (method === undefined) {
if (action && action != '#') {
if (usePjax) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Can be a one-liner.

@arogachev

arogachev Jan 2, 2017

Member

Can be a one-liner.

} else if ($e.is(':submit') && $form.length) {
if (usePjax) {
$form.on('submit',function(e){

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Code style.

@arogachev

arogachev Jan 2, 2017

Member

Code style.

var oldMethod,
oldAction,
newForm = !$form.length;
if (!newForm) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • It's better to have shorter part of if first so it's easier to read.
  • Have put the part for existing form under if check, We do not need any oldMethod / oldAction processing in case of a new form (it will be deleted after submit anyways).
@arogachev

arogachev Jan 2, 2017

Member
  • It's better to have shorter part of if first so it's easier to read.
  • Have put the part for existing form under if check, We do not need any oldMethod / oldAction processing in case of a new form (it will be deleted after submit anyways).
@@ -219,9 +216,10 @@ window.yii = (function ($) {
}
if (!/(get|post)/i.test(method)) {
$form.append($('<input/>', {name: '_method', value: method, type: 'hidden'}));
method = 'POST';
method = 'post';

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • Use lower case.
  • With putting part for the existing form in if check we also need to explicitly set method attribute.
@arogachev

arogachev Jan 2, 2017

Member
  • Use lower case.
  • With putting part for the existing form in if check we also need to explicitly set method attribute.
}
if (!/(get|head|options)/i.test(method)) {
if (/post/i.test(method)) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Having check against GET, HEAD and OPTIONS methods is redundant here, because method will be only get or post at this point (in case of other methods, for example HEAD variable method will be assigned to post in previous check).

@arogachev

arogachev Jan 2, 2017

Member

Having check against GET, HEAD and OPTIONS methods is redundant here, because method will be only get or post at this point (in case of other methods, for example HEAD variable method will be assigned to post in previous check).

@@ -232,49 +230,41 @@ window.yii = (function ($) {
var activeFormData = $form.data('yiiActiveForm');
if (activeFormData) {
// remember who triggers the form submission. This is used by yii.activeForm.js
// Remember the element triggered the form submission. This is used by yii.activeForm.js.

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • I think who means refering to the person, so it's not suitable here.
  • Multiple sentences, added dot at the end of the last sentence.
@arogachev

arogachev Jan 2, 2017

Member
  • I think who means refering to the person, so it's not suitable here.
  • Multiple sentences, added dot at the end of the last sentence.
activeFormData.submitObject = $e;
}
// temporarily add hidden inputs according to data-params
if (params && $.isPlainObject(params)) {
$.each(params, function (idx, obj) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

idx and obj are not acceptable here. Params are stored in name - value structure.

@arogachev

arogachev Jan 2, 2017

Member

idx and obj are not acceptable here. Params are stored in name - value structure.

if (usePjax) {
$form.on('submit',function(e){
$form.on('submit', function (e) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • Code style problems.
  • Missing semicolon.
@arogachev

arogachev Jan 2, 2017

Member
  • Code style problems.
  • Missing semicolon.
});
}
var oldMethod = $form.attr('method');

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • Moved to the if check (see above).
  • Default value of oldAction variable can be undefined. I don't see any specific reasons to use null here.
@arogachev

arogachev Jan 2, 2017

Member
  • Moved to the if check (see above).
  • Default value of oldAction variable can be undefined. I don't see any specific reasons to use null here.
});
}
$.when($form.data('yiiSubmitFinalizePromise')).then(function () {
if (newForm) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

We don't need any additional processing if the form was new and temporary. Because it will be deleted anyways, we can just use premature return.

@arogachev

arogachev Jan 2, 2017

Member

We don't need any additional processing if the form was new and temporary. Because it will be deleted anyways, we can just use premature return.

// remove the temporarily added hidden inputs
if (params && $.isPlainObject(params)) {
$.each(params, function (idx, obj) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

idx and obj are not appropriate for params stored in name - value structure.

@arogachev

arogachev Jan 2, 2017

Member

idx and obj are not appropriate for params stored in name - value structure.

window.location = action;
}
if (isValidAction) {
usePjax ? $.pjax.click(event, pjaxOptions) : window.location.assign(action);

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Use assign method for easier stub creation. See http://stackoverflow.com/a/40495796/4323648. It has no difference - https://developer.mozilla.org/ru/docs/Web/API/Location/assign.

@arogachev
}
$form.trigger('submit');
}
return;
}
var newForm = !$form.length;
if (newForm) {
if (!action || !/(^\/|:\/\/)/.test(action)) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

This condradicts with other action checks:

action && action != '#';

Also it seems useless. What was the purpose of it? To check if the url is valid? Then this check is not enough.

@arogachev

arogachev Jan 2, 2017

Member

This condradicts with other action checks:

action && action != '#';

Also it seems useless. What was the purpose of it? To check if the url is valid? Then this check is not enough.

This comment has been minimized.

@samdark

samdark Jan 4, 2017

Member

It's there from 2013. Was implemented by Qiang and, indeed, looks like URL validation.

@samdark

samdark Jan 4, 2017

Member

It's there from 2013. Was implemented by Qiang and, indeed, looks like URL validation.

@@ -284,67 +274,185 @@ window.yii = (function ($) {
}
var pairs = url.substring(pos + 1).split('#')[0].split('&'),
params = {},
pair,

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Predeclaring of pair andi` variables seems redundant.

@arogachev

arogachev Jan 2, 2017

Member

Predeclaring of pair andi` variables seems redundant.

for (i = 0; i < pairs.length; i++) {
pair = pairs[i].split('=');
for (var i = 0, len = pairs.length; i < len; i++) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Micro-optimization - calculate len only once, at the very beginning of the loop.

@arogachev

arogachev Jan 2, 2017

Member

Micro-optimization - calculate len only once, at the very beginning of the loop.

params[name].push(value || '');
} else {
params[name] = value || '';
if (!name.length) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Got rid of the extra enclosure of if checks with premature return for better readability.

@arogachev

arogachev Jan 2, 2017

Member

Got rid of the extra enclosure of if checks with premature return for better readability.

pub.initModule(this);
}
});
if (module.isActive !== undefined && !module.isActive) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Got rid of the extra enclosure of if checks with premature return for better readability.

@arogachev

arogachev Jan 2, 2017

Member

Got rid of the extra enclosure of if checks with premature return for better readability.

},
init: function () {
initCsrfHandler();
initRedirectHandler();
initScriptFilter();
initAssetFilters();

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Partial fix for #13299.

@arogachev

arogachev Jan 2, 2017

Member

Partial fix for #13299.

}
};
function initCsrfHandler() {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • Reordered private functions to match the order of according calls in init method.
  • Moved private utility functions to the end.
@arogachev

arogachev Jan 2, 2017

Member
  • Reordered private functions to match the order of according calls in init method.
  • Moved private utility functions to the end.
function initRedirectHandler() {
// handle AJAX redirection
$(document).ajaxComplete(function (event, xhr, settings) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Unused parameter.

@arogachev

arogachev Jan 2, 2017

Member

Unused parameter.

$(document).ajaxComplete(function () {
var styleSheets = [];
$('link[rel=stylesheet]').each(function () {
var url = getAbsoluteUrl(this.href);

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member
  • Fix for #13310.
  • Converted check to one-liner.
@arogachev

arogachev Jan 2, 2017

Member
  • Fix for #13310.
  • Converted check to one-liner.
}
return pub;
})(jQuery);
})(window.jQuery);

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

It's better to use window here. Plus the consistency with other JS files.

@arogachev

arogachev Jan 2, 2017

Member

It's better to use window here. Plus the consistency with other JS files.

jQuery(function () {
yii.initModule(yii);
window.jQuery(function () {
window.yii.initModule(window.yii);

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Variable yii was set using window.yii, but then accessed with just yii. This is also hard to test.

@arogachev

arogachev Jan 2, 2017

Member

Variable yii was set using window.yii, but then accessed with just yii. This is also hard to test.

@@ -17,13 +17,13 @@
* A module may be structured as follows:
*
* ```javascript
* yii.sample = (function($) {
* window.yii.sample = (function($) {

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Added window to better reflect Yii modules.

@arogachev

arogachev Jan 2, 2017

Member

Added window to better reflect Yii modules.

@@ -782,7 +782,7 @@ protected function getDispositionHeaderValue($disposition, $attachmentName)
*
* ```javascript
* $document.ajaxComplete(function (event, xhr, settings) {
* var url = xhr.getResponseHeader('X-Redirect');
* var url = xhr && xhr.getResponseHeader('X-Redirect');

This comment has been minimized.

@arogachev

arogachev Jan 2, 2017

Member

Sync with #10974.

@arogachev

arogachev Jan 2, 2017

Member

Sync with #10974.

@arogachev

This comment has been minimized.

Show comment
Hide comment
@arogachev

arogachev Jan 2, 2017

Member

Seems like current Code Climate ESLint config does not allow one-liners with expressions?

usePjax ? $.pjax.click(event, pjaxOptions) : window.location.assign(action);

Also what's wrong with:

if (window.confirm(message)) {
}

?

Member

arogachev commented Jan 2, 2017

Seems like current Code Climate ESLint config does not allow one-liners with expressions?

usePjax ? $.pjax.click(event, pjaxOptions) : window.location.assign(action);

Also what's wrong with:

if (window.confirm(message)) {
}

?

@arogachev arogachev referenced this pull request Jan 2, 2017

Open

Have at least basic tests for js #12840

6 of 7 tasks complete
@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 2, 2017

Member

don't take code climate too serious we made a lot of adjustments to make it fit in PHP. We can adjust it for JS later if needed.

Member

cebe commented Jan 2, 2017

don't take code climate too serious we made a lot of adjustments to make it fit in PHP. We can adjust it for JS later if needed.

}
$form.trigger('submit');
}
return;
}
var newForm = !$form.length;
if (newForm) {
if (!action || !/(^\/|:\/\/)/.test(action)) {

This comment has been minimized.

@samdark

samdark Jan 4, 2017

Member

It's there from 2013. Was implemented by Qiang and, indeed, looks like URL validation.

@samdark

samdark Jan 4, 2017

Member

It's there from 2013. Was implemented by Qiang and, indeed, looks like URL validation.

if (newForm) {
$form.remove();
}
if (oldAction !== undefined) {

This comment has been minimized.

@samdark

samdark Jan 4, 2017

Member

Can't we check as if (oldAction)?

@samdark

samdark Jan 4, 2017

Member

Can't we check as if (oldAction)?

This comment has been minimized.

@arogachev

arogachev Jan 5, 2017

Member

Technically - yes, because oldAction is set only when isValidAction is truthy:

if (isValidAction) {
    oldAction = $form.attr('action');
    $form.attr('action', action);
}

And isValidAction disallows all falsy values and additionally anchor (#):

isValidAction = action && action !== '#',

But checking for undefined is more clear IMO.

@arogachev

arogachev Jan 5, 2017

Member

Technically - yes, because oldAction is set only when isValidAction is truthy:

if (isValidAction) {
    oldAction = $form.attr('action');
    $form.attr('action', action);
}

And isValidAction disallows all falsy values and additionally anchor (#):

isValidAction = action && action !== '#',

But checking for undefined is more clear IMO.

if (!name.length) {
continue;
}
if (params[name] === undefined) {

This comment has been minimized.

@samdark

samdark Jan 4, 2017

Member

Could we check for if (params[name]) here?

@samdark

samdark Jan 4, 2017

Member

Could we check for if (params[name]) here?

This comment has been minimized.

@arogachev

arogachev Jan 5, 2017

Member

No. See the next line:

params[name] = value || '';

If the value was set to an empty string if (params[name]) { } check will evaluate to false with existing key.

As an alternative we can use in or hasOwnProperty.

@arogachev

arogachev Jan 5, 2017

Member

No. See the next line:

params[name] = value || '';

If the value was set to an empty string if (params[name]) { } check will evaluate to false with existing key.

As an alternative we can use in or hasOwnProperty.

Show outdated Hide outdated framework/assets/yii.js
Show outdated Hide outdated framework/assets/yii.js
Show outdated Hide outdated framework/assets/yii.js
@samdark

samdark approved these changes Jan 5, 2017

@samdark samdark requested a review from SilverFire Jan 14, 2017

* @returns {string}
*/
getBaseCurrentUrl: function () {
return window.location.protocol + '//' + window.location.host;

This comment has been minimized.

@SilverFire

SilverFire Jan 14, 2017

Member

Will work incorrect when port is defined, e.g. localhost:8080
It will be good to cover it with a test

@SilverFire

SilverFire Jan 14, 2017

Member

Will work incorrect when port is defined, e.g. localhost:8080
It will be good to cover it with a test

This comment has been minimized.

@arogachev

arogachev Jan 14, 2017

Member

window.location.host includes port.

For example for given url http://localhost:8000/ the result of:

window.location.protocol + '//' + window.location.host

is http://localhost:8000 which is correct. So this is not a problem.

@arogachev

arogachev Jan 14, 2017

Member

window.location.host includes port.

For example for given url http://localhost:8000/ the result of:

window.location.protocol + '//' + window.location.host

is http://localhost:8000 which is correct. So this is not a problem.

This comment has been minimized.

@SilverFire

SilverFire Jan 14, 2017

Member

I was wrong, mixed up host and hostname properties.

@SilverFire

SilverFire Jan 14, 2017

Member

I was wrong, mixed up host and hostname properties.

@SilverFire

This comment has been minimized.

Show comment
Hide comment
@SilverFire

SilverFire Jan 14, 2017

Member

Incredible job done, we highly appreciate your efforts. Thank you very much, @arogachev, for making Yii better!

Member

SilverFire commented Jan 14, 2017

Incredible job done, we highly appreciate your efforts. Thank you very much, @arogachev, for making Yii better!

@samdark samdark self-assigned this Jan 25, 2017

@samdark samdark merged commit 37f19a0 into yiisoft:master Jan 25, 2017

1 check failed

codeclimate 4 new issues (8 fixed)
Details
@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Jan 25, 2017

Member

Merged. Thank you very much for this wonderful pull request.

Member

samdark commented Jan 25, 2017

Merged. Thank you very much for this wonderful pull request.

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