Preparation for #8452: 'jquery' package extraction #13839

klimov-paul · 2017-03-23T13:43:59Z

Q	A
Is bugfix?	no
New feature?	yes
Breaks BC?	yes
Tests pass?	yes
Fixed issues	#12588, preparing for #8452

'jquery' package has been extracted as preparation for yii2-jquery repo extraction, which will allow to create requirements free version of Yii2 and make composer-asset-plugin installation to be optional.

…tion

rob006 · 2017-03-23T14:04:42Z

Can't we just use jQuery from CDN by default?

klimov-paul · 2017-03-23T14:07:54Z

Can't we just use jQuery from CDN by default?

It makes no difference. PHP framework should not be bound to any JavaScript framework.
It should be developer choice if he wishes to use 'JQuery', 'ReactJS', 'Angular' or anything else.

rob006 · 2017-03-23T14:22:26Z

It makes no difference.

It fixes main issue - you don't need asset plugin anymore and you can install Yii without any frontend dependencies.

I agree that framework-agnostic approach is generally good thing, but it looks like overkill and unnecessary complication. If I want write extension which extends GridView, which class I should use as a base class?

klimov-paul · 2017-03-23T14:47:57Z

I agree that framework-agnostic approach is generally good thing, but it looks like overkill and unnecessary complication.

Following your arguments, should not we bind Yii2 to Twitter Bootstrap as well? Why keep yii2-bootstrap as a separated repo? Does not it produce an 'overkill and unnecessary complication'?

If I want write extension which extends GridView, which class I should use as a base class?

It depends on what you want to do: you may use yii\grid\GridView and provide your own JS layer for its filter - or do not use any JS layer, but a regular HTML form submit. Or you can extend yii\jquery\GridView and inherit related JQuery code with it.

I am fixing the issues, which are opened and which core team has agreed to be fixed. If you wish to debate on necessity of such changes use the related issues for that: #8452, #11792, #10557, #8709, #9525.
When you are able to convince @WadeShuler, @leandrogehlen, @graphicmist, @fabiocarneiro and others that thier arguments are unfounded I will discard this PR.

klimov-paul · 2017-03-23T15:06:47Z

This PR fixes #12588

rob006 · 2017-03-23T15:14:52Z

Following your arguments [...]

Please, be constructive...

It depends on what you want to do [...]

My extension is not related to JS at all so I may use any of this class. But if I use yii\grid\GridView as a base class people who want use jQuery stuff in GridView will not be able to use my widget. If I use yii\jquery\GridView, I will force users to download all this frontend dependencies. So basically I need create and maintain 2 extensions instead of 1.

I am fixing the issues, which are opened and which core team has agreed to be fixed. If you wish to debate on necessity of such changes use the related issues for that: #8452, #11792, #10557, #8709, #9525.

Did you at least read these issues before linking? They are mostly about installing frontend dependencies, which may be solved by using CDN fallback. Some of them even proposing using CDN for jQuery.

samdark · 2017-03-23T15:22:15Z

Hmm... @rob006 has a point about having two separate GridViews...

samdark · 2017-03-23T15:23:15Z

framework/UPGRADE.md

+  You should use `yii\jquery\*` package classes instead, to make old code function as before. E.g. use `yii\jquery\ActiveForm` instead
+  of `yii\widgets\ActiveForm`, `yii\jquery\GridView` instead of `yii\grid\GridView` and so on.
+
+* Assets `yii\web\JqueryAsset`, `yii\web\YiiAsset`, `yii\validators\ValidationAsset`, `yii\captcha\CaptchaAsset` have been


I was planning extracting Captcha in its own package.

dynasource · 2017-03-23T19:13:30Z

I agree with @rob006 , I'm positive towards disentanglement of parts, but I dont like the overuse of inheritance in the framework. This is not exactly favoring 'composition' over inheritance and limits flexibility.

samdark · 2017-03-23T20:38:53Z

@dynasource any ideas on how to do it better?

dynasource · 2017-03-23T21:13:44Z

like I said, encapsulate the logic in dependencies instead of inheritance. All these JS parts deserve their own classes and should not pollute the inheritance tree.

dynasource · 2017-03-23T21:18:08Z

this is also an example of GridView has jQuery instead of GridView is jQuery
https://en.wikipedia.org/wiki/Composition_over_inheritance#Benefits

samdark · 2017-03-23T22:30:14Z

I understand the theory. I was asking about its practical application for this particular case.

samdark · 2017-03-24T16:49:19Z

As I said above it could be a behavior.

I like the idea. It could be configured via container as well...

samdark · 2017-03-24T16:50:37Z

Anyway both yii2-captcha and yii2-jquery should be composed inside 2.1 branch before using GIT tree subsplit.

Why? I've successfully extracted https://github.com/yiisoft/yii2-maskedinput without doing a subsplit...

samdark · 2017-03-24T16:51:31Z

The main question is where JQuery dependent part of Captcha widget should be located in the end: at yii2-captcha or at yii2-jquery?

At yii2-captcha but it should be a behavior or a separate class. Not a widget inherited from Captcha.

samdark · 2017-03-24T16:52:57Z

Client validators can be extracted into a separated classes mapped to the existing server-side validators.
But they will be almost empty containing a 3-4 lines of code. Still I can adjust code in this way if you like.

I think it would make perfect sense.

klimov-paul · 2017-03-27T12:10:35Z

Why? I've successfully extracted https://github.com/yiisoft/yii2-maskedinput without doing a subsplit

Subsplit is better as it will preserve the git history.

At yii2-captcha but it should be a behavior or a separate class. Not a widget inherited from Captcha.

So it will, but my question still stands:

where JQuery dependent part of Captcha widget should be located in the end: at yii2-captcha or at yii2-jquery?

If it will be located at yii2-captcha it will mean that this package will be dependent on yii2-jquery and can not be used without it. Thus it will be impossible to create a CAPTCHA solution without installing jQuery.
If it will be located yii2-jquery it will be dependent on yii2-captcha, thus you'll have to install captcha before using jQuery even if you do not need it.
The solid way to resolve it will be creating a 3rd package, which will depend on both yii2-captcha and yii2-jquery, but this may be an overcomplication.
Another solution can be putting JS-dependent part at one of yii2-captcha or yii2-jquery without explicit dependency ('suggets' composer section can be used for example), but this can be inconsistent.

samdark · 2017-03-27T12:59:48Z

About jQuery-related part of CAPTCHA, I prefer to rewrite JS part of it without using jQuery. yii.captcha.js isn't too complicated to do it.

leandrogehlen · 2017-03-27T14:12:38Z

My suggestion.

yii2-widgets (base widigets without js or css dependencies)
yii2-jquery (GridViewBehavior, and assets and capcha) everthing that use jquery

and yii2-jquery must be separated from core framework

samdark · 2017-03-27T14:34:47Z

@leandrogehlen separating widgets completely isn't the goal of this pull request.

leandrogehlen · 2017-03-27T15:37:32Z

Ok, then , i think that the folder grid should be moved to widgets and removed assets dependencies from this classes, this way we can write a grid with jQuery and another with Angular , thus each extension can write a grid behavior with js library desired

klimov-paul · 2017-03-27T18:58:28Z

Solution has been refactored:

Client script for ActiveForm, GridView and Captcha extracted into a behaviors.
Client validation composition chnaged to use class map and special ClientValidator class
captcha related code packed under framework/captcha

samdark · 2017-03-27T19:29:32Z

Looks good after quick glance on it. Need to check it in depth though.

schmunk42 · 2017-09-05T14:48:28Z

I took a look at this and actually, the PR was looking good, at least no assets folder anymore in the framework.

I tried to merge against 2.1, but it looked to me like there were a lot of clientXyz functions added again?!

I think also that all the remaining asset-bundles should be removed from the core, but one step after the other...

Can this be made mergeable again?

klimov-paul · 2017-09-05T15:06:21Z

Can this be made mergeable again?

At the present state I do not have time for this. I need to finish #14701 and underlying tasks, which depends on it. Otherwise, it will likely end in conflicts anyway.

Most likely, I will re-create this PR from scratch afterwards.

Since, at the present state, core team has no interest on reviewing this or attend to the underlying issue, it is not in hurry. Even if I merge this changeset, it will solves nothing until the jquery and captcha reositories will be split. While I do not have enough permissions to create a repositories under yiisoft, I am unable to to do it myself.
Without major core team suport this PR moves nowhere anyway.

schmunk42 · 2017-09-05T15:36:45Z

it will solves nothing until the jquery and captcha reositories will be split. While I do not have enough permissions to create a repositories under yiisoft, I am unable to to do it myself.

100% agreed! @samdark Could you create those repos for 2.1 (yii2-jquery and yii2-captcha; also yii2-pjax)? Those need to be moved out.

samdark · 2017-09-06T11:06:31Z

PJAX is to be removed so I haven't created a repo for it.

klimov-paul · 2017-09-21T17:04:56Z

Migrated to #14865

klimov-paul added 7 commits March 21, 2017 17:49

jquery package extraction started

fe10494

ActiveForm and GridView of jquery package provided

618a04f

yii\jquery\Captcha extracted

3c1ddf1

Client validation composition reworked

d25485c

Unit tests fix

53e6e91

client validation composition for CaptchaValidator fixed

4846fa9

UPGRADE note about jquery package provided

ec72a28

klimov-paul added severity:BC breaking Either breaks backwards compatibility or fixed previously made breakage status:under discussion type:enhancement labels Mar 23, 2017

klimov-paul added this to the 2.1.0 milestone Mar 23, 2017

Merge branch '2.1' of github.com:yiisoft/yii2 into 8452-jquery-extrac…

ddcef80

…tion

samdark reviewed Mar 23, 2017

View reviewed changes

samdark requested review from dynasource, cebe, mdomba, SilverFire and arogachev March 23, 2017 15:28

yii\jquery\GridViewBehavior extracted

a6a74ec

client validato classes extracted

b07fceb

klimov-paul added 4 commits March 27, 2017 18:48

CaptchaClientBehavior extracted

094d268

GridViewBehavior renamed to GridViewClientBehavior

2637ebd

ActiveFormClientScriptBehavior extracted

d10ac3a

client-script widget behaviors renamed

19e37c6

klimov-paul mentioned this pull request Apr 12, 2017

Refactor Captcha widget and write tests. #13799

Closed

klimov-paul mentioned this pull request Jul 24, 2017

Re-write Gii JavaScript not to use jQuery yiisoft/yii2-gii#282

Open

klimov-paul mentioned this pull request Aug 9, 2017

Why still using deprecated jQuery(document).ready()? #14620

Closed

klimov-paul mentioned this pull request Sep 5, 2017

Moving from bower to npm #14297

Closed

schmunk42 mentioned this pull request Sep 12, 2017

Yii2 as Microframework: Add Explanation how to setup Tests #14788

Open

klimov-paul mentioned this pull request Sep 21, 2017

Fix #8452 Jquery Extraction (attempt 2) #14865

Merged

klimov-paul closed this Sep 21, 2017

schmunk42 mentioned this pull request Dec 19, 2019

Asset management in Yii 2.1 yiisoft/assets#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preparation for #8452: 'jquery' package extraction #13839

Preparation for #8452: 'jquery' package extraction #13839

klimov-paul commented Mar 23, 2017 •

edited

Loading

rob006 commented Mar 23, 2017

klimov-paul commented Mar 23, 2017

rob006 commented Mar 23, 2017

klimov-paul commented Mar 23, 2017 •

edited

Loading

klimov-paul commented Mar 23, 2017

rob006 commented Mar 23, 2017

samdark commented Mar 23, 2017

samdark Mar 23, 2017

dynasource commented Mar 23, 2017

samdark commented Mar 23, 2017

dynasource commented Mar 23, 2017

dynasource commented Mar 23, 2017

samdark commented Mar 23, 2017

samdark commented Mar 24, 2017

samdark commented Mar 24, 2017

samdark commented Mar 24, 2017

samdark commented Mar 24, 2017

klimov-paul commented Mar 27, 2017

samdark commented Mar 27, 2017

leandrogehlen commented Mar 27, 2017 •

edited

Loading

samdark commented Mar 27, 2017 •

edited

Loading

leandrogehlen commented Mar 27, 2017

klimov-paul commented Mar 27, 2017

samdark commented Mar 27, 2017

schmunk42 commented Sep 5, 2017

klimov-paul commented Sep 5, 2017

schmunk42 commented Sep 5, 2017

samdark commented Sep 6, 2017

klimov-paul commented Sep 21, 2017

Preparation for #8452: 'jquery' package extraction #13839

Preparation for #8452: 'jquery' package extraction #13839

Conversation

klimov-paul commented Mar 23, 2017 • edited Loading

rob006 commented Mar 23, 2017

klimov-paul commented Mar 23, 2017

rob006 commented Mar 23, 2017

klimov-paul commented Mar 23, 2017 • edited Loading

klimov-paul commented Mar 23, 2017

rob006 commented Mar 23, 2017

samdark commented Mar 23, 2017

samdark Mar 23, 2017

Choose a reason for hiding this comment

dynasource commented Mar 23, 2017

samdark commented Mar 23, 2017

dynasource commented Mar 23, 2017

dynasource commented Mar 23, 2017

samdark commented Mar 23, 2017

samdark commented Mar 24, 2017

samdark commented Mar 24, 2017

samdark commented Mar 24, 2017

samdark commented Mar 24, 2017

klimov-paul commented Mar 27, 2017

samdark commented Mar 27, 2017

leandrogehlen commented Mar 27, 2017 • edited Loading

samdark commented Mar 27, 2017 • edited Loading

leandrogehlen commented Mar 27, 2017

klimov-paul commented Mar 27, 2017

samdark commented Mar 27, 2017

schmunk42 commented Sep 5, 2017

klimov-paul commented Sep 5, 2017

schmunk42 commented Sep 5, 2017

samdark commented Sep 6, 2017

klimov-paul commented Sep 21, 2017

klimov-paul commented Mar 23, 2017 •

edited

Loading

klimov-paul commented Mar 23, 2017 •

edited

Loading

leandrogehlen commented Mar 27, 2017 •

edited

Loading

samdark commented Mar 27, 2017 •

edited

Loading