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

bower to npm in 2.1 #221

Closed
alexantr opened this issue Oct 20, 2017 · 36 comments
Closed

bower to npm in 2.1 #221

alexantr opened this issue Oct 20, 2017 · 36 comments
Labels
status:ready for adoption Feel free to implement this issue.

Comments

@alexantr
Copy link

Since bower got deprecated status will be good idea to replace bower packages (bootstrap and popper.js) with npm packages.

@samdark samdark added this to the 2.1 milestone Oct 20, 2017
@samdark
Copy link
Member

samdark commented Oct 20, 2017

Agree.

@schmunk42
Copy link
Contributor

Yes, we should do that, this will make the transition to a new asset installation system also much easier.

Related:

@samdark samdark added the status:ready for adoption Feel free to implement this issue. label Oct 20, 2017
@schmunk42
Copy link
Contributor

Here's a proposal how it could be done in 2.1: #212

It would be nice if the above PR could be merged into 2.1 to move ahead with the transformation.

@beroso
Copy link

beroso commented Jan 18, 2018

Bootstrap 4 can be installed via composer too. It's suitable?

@samdark
Copy link
Member

samdark commented Jan 19, 2018

Probably yes.

@rob006
Copy link

rob006 commented Jan 19, 2018

I think we should stick to one repository for frontend packages to avoid duplicated and incompatible dependencies. TB may be available at packagist, but many widgets that depends on bootstrap does not support installation via composer. If we install bootstrap by composer and package X (which depends on bootstrap) by npm, we will get two different bootstrap packages.

@schmunk42
Copy link
Contributor

What's the merge/split policy for yii2-bootstrap? For yii2 there we'll have to wait for 2.0.14 before there'll be a removal of bower-assets.

Can we remove the dependencies here in the 2.1 branch or do we have to wait for the core framework?

In 2.1 there should only be a packages.json file for assets, since bower is deprecated and npm won't be manageable well with composer as laid out here.

PS: Bootstrap also requires popper.js for example.

@samdark
Copy link
Member

samdark commented Jan 19, 2018

I'd say:

  1. Fix as many issues and merge as many PRs as possible into bootstrap3 (master) branch.
  2. Make a release.
  3. Merge it into 2.1.
  4. Make 2.1 primary branch (alternatively we can make 2.0 branch and move 2.1 into master).
  5. Fix bootstrap4 things.
  6. Release.

@schmunk42
Copy link
Contributor

schmunk42 commented Jan 19, 2018

The versions are starting to confuse me :)

The question is, if there really should be a 2.1 version of this repo with Bootstrap 4 - how about yii2-bootstrap4? The major version change should not be "hidden". I somehow doubt that this can be easily upgraded. For sure the widgets itself can be, but you usually have a lot more BS-dependent code in your app.

Moreover - and this applies also to other extensions - a 2.1 version of the extension don't have to be necessarily connected to framework version 2.1 - which might be not very obvious.

So a version of this repo which uses a very different asset management and BS4 should rather be a 3.0 or yii2.1-boostrap4:1.0 - confusing ... isn't it?

@samdark
Copy link
Member

samdark commented Jan 19, 2018

3.0 sounds OK to me.

@rob006
Copy link

rob006 commented Jan 20, 2018

I thought that 2 is fixed as a prefix for version of all yii2 packages. We already have 2.1 releases for packages compatible with Yii 2.0, why use 3.0 in this case?

@adiramardiani
Copy link

Agree 3.0 for bootstrap 4
And 2.x for bootstrap 3

@rob006
Copy link

rob006 commented Jan 20, 2018

3.0 release contradicts with existing versioning policy: https://github.com/yiisoft/yii2/blob/master/docs/internals/versions.md

@samdark
Copy link
Member

samdark commented Jan 20, 2018

That's framework version policy.

@rob006
Copy link

rob006 commented Jan 20, 2018

All official extensions seems to follow it. It will be really confusing, if we make exception only for one extension.

@samdark
Copy link
Member

samdark commented Jan 20, 2018

What do you propose instead?

@rob006
Copy link

rob006 commented Jan 20, 2018

Release it as 2.1.

@schmunk42
Copy link
Contributor

schmunk42 commented Feb 13, 2018

Bower support has been dropped for v4, btw: twbs/bootstrap#24590, twbs/bootstrap#23568

@schmunk42
Copy link
Contributor

schmunk42 commented Mar 9, 2018

I took a deeper look on the asset installation procedure with npm-assets with AP and CAP, and found rather worrying things.


As outlined in https://github.com/yiisoft/yii2/issues/14862 npm is capable of installing multiple version of the same package, which leads to (unsolvable) errors with some packages, like ie. mermaid, see also hiqdev/asset-packagist#69

The same issue pops also up in this combination:

    "require": {
        "npm-asset/selectize.js": "0.12.*",
        "npm-asset/fastlog": "1.*"
    },

The unsolvable dependency is minimist, which has over 9000 (not a joke) dependents. If there's one constraint mismatch, you won't be able to install.

Wan't it even worse? Checkout https://www.npmjs.com/package/lodash - over 63.000 dependents, like 10% of the ecosystem.

And nobody will care about this problem, since it isn't one when using npm!


Moreover, with npm many packages do not have dist files in the repo all-the-time, ie. if you get dev-master of a package like mermaid, you do not have any usable files at all.


That's not all, when installing npm-asset/bootstrap:4.* you won't get the one dependency you actually really need: popper.js, since it's a peerDependency in npm.

TL;dr

We should not transistion to npm-asset handled by composer in any way (at least by default). This will just show bad-practice to developers - even if this is a very painful decision.

We should remove all npm-asset/... from composer.json files.

This will break some things in the first place, like mentioned from @klimov-paul in https://github.com/yiisoft/yii2/issues/14862#issuecomment-369224200 - but IMHO it is better to break things now and require npm/yarn in this early development stage of 2.1, than to ignore this and worry later.

It's doomed to failure, if it stays like it is.

@samdark
Copy link
Member

samdark commented Mar 9, 2018

Makes sense. Seems we have to do it :(

@fcaldarelli
Copy link
Member

@schmunk42 What will change by developer point of view using directly npm instead npm-asset?

@schmunk42
Copy link
Contributor

You'll need to install npm or yarn. But that's the only con IMHO. The native solution is much faster and less error-prone.

@fcaldarelli
Copy link
Member

@schmunk42 It is ok. This is an irrilevant con if compared to pros.

@alexantr
Copy link
Author

It would be great to use npm instead workarounds like asset-packagist.

Just one more command.

composer require yiisoft/yii2-bootstrap
npm i --save bootstrap

@schmunk42
Copy link
Contributor

@alexantr Have a look https://github.com/fxpio/foxy - it would (optinally) automate this process

@alexantr
Copy link
Author

@schmunk42 It seems nice

Foxy retrieves the list of all Composer dependencies to inject the asset dependencies in the file package.json, and leaves the execution of the analysis, validation and downloading of the libraries to NPM or Yarn. Therefore, no VCS Repository of Composer is used for analyzing the asset dependencies, and you keep the performance of native package manager used.

@cebe
Copy link
Member

cebe commented Mar 12, 2018

As outlined in yiisoft/yii2#14862 npm is capable of installing multiple version of the same package, which leads to (unsolvable) errors with some packages, like i.e. mermaid, see also hiqdev/asset-packagist#69

the PHP based stack is not meant to provide the full functionality that npm has. It is meant to be used for simple setups, e.g. if you fetch bootstrap, jquery and 1-2 other libs for a simple site or application.
If your application depends on a package that only works with native npm, you should be able to switch the stack and use npm directly.

Given the mess that npm and javascript in general currently is, we need a way to use Yii for developing a website without the need to deal with all the complexity introduced by the nodejs/npm stack.

@schmunk42
Copy link
Contributor

Given the mess that npm and javascript in general currently is, we need a way to use Yii for developing a website without the need to deal with all the complexity introduced by the nodejs/npm stack.

I agree to this statement, but on the other hand we also need to show the "correct" way to the developer. What you've said is feasible for jQuery & Bootstrap, because there are core AssetBundles for those currently - and you can depend on them.

But for anything else you run into problems, take https://github.com/Insolita/yii2-adminlte-widgets for example - they require AdminLTE, but there's no official core asset-bundle for it, so you can't depend on anything (and if you would, some people might want to exchange the PHP asset-bundle). It also makes no sense to provide your own, because in this case there is ie. our widely used extension. A dependency would only make sense on a CSS/JS level.

While I don't support a CDN by default, how about the following (would it be possible?):

  • assume you install yii2-app (2.1) along with yii2-jquery and yii2-bootstrap
  • the AssetBundles there point to @node_modules/... (which is a local, published path, by default)

Could we reconfigure @node_modules to http://cdn.node-modules.org?

@samdark
Copy link
Member

samdark commented Mar 12, 2018

Using CDN by default is a no go.

@schmunk42
Copy link
Contributor

Sure, the question is, if a developer could install an application template - and then manually set the mentioned alias, like moving from local filesystem to CDN, just by changing the "include path".

An additional alternative would be an auto-built composer package, with the required JS/CSS files.

@samdark
Copy link
Member

samdark commented Mar 14, 2018

Isn't it what we have now with asset-packagist? It's auto-built composer package.

@alexantr
Copy link
Author

alexantr commented Apr 4, 2018

Need a composer repository like asset-packagist but only with npm dist builds and normalized dependencies 😉

@schmunk42
Copy link
Contributor

@alexantr Related? hiqdev/asset-packagist#67

A problem with npm is that you can't rely on getting dist files for several commits.
What do you mean with normalized dependencies? Might make sense to continue this point at asset-packagist.

@alexantr
Copy link
Author

alexantr commented Apr 4, 2018

Just want to use npm simply like bower.

@schmunk42
Copy link
Contributor

Just want to use npm simply like bower.

Yeah 😺 I would love to do that also.

Now that bower is gone, I miss it. Bower was actually ideal for the way assets were handled in Yii2. But it is like it is, we'll have to live with npm and its issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready for adoption Feel free to implement this issue.
Projects
None yet
Development

No branches or pull requests

8 participants