Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

chore(*): get rid of Bower in favor of Yarn aliases & checked-in packages #16385

Closed
wants to merge 1 commit into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Dec 27, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Build change.

What is the current behavior? (You can also link to an open issue here)

Bower is used to install jQuery & Closure Compiler-related packages.

What is the new behavior (if this is a feature change)?

Most packages are installed from the npm registry, Yarn aliases are used to manage multiple jQuery versions. ng-closure-compiler was checked in to the repository.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

Bower was used to install multiple versions of jQuery which is now handled
using Yarn aliases. The remaining two packages, closure-compiler and
ng-closure-compiler were installed from zip files which is not supported by Yarn
(see yarnpkg/yarn#1483); the first of them exists
on npm as the google-closure-compiler but only versions newer than we used are
published and they don't work with ng-closure-compiler so - instead - both were
checked in to the repository.

Fixes #16268
Fixes #14961
Ref yarnpkg/yarn#1483

…ages

Bower was used to install multiple versions of jQuery which is now handled
using Yarn aliases. The remaining two packages, closure-compiler and
ng-closure-compiler were installed from zip files which is not supported by Yarn
(see yarnpkg/yarn#1483); the first of them exists
on npm as the google-closure-compiler but only versions newer than we used are
published and they don't work with ng-closure-compiler so - instead - both were
checked in to the repository.

Fixes angular#16268
Fixes angular#14961
Ref yarnpkg/yarn#1483
@@ -4,7 +4,7 @@ var path = require('canonical-path');
/**
* dgService getVersion
* @description
* Find the current version of the bower component (or node module)
* Find the current version of the node module
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this function is only used to get node modules (afaict it does), we can simplify the implementation and remove the second and third arguments. I think this happened already when we moved most of the bower stuff to node.

@Narretz Narretz added this to the 1.7.0 milestone Dec 29, 2017
[yarn-install]: https://yarnpkg.com/en/docs/install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what changed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new line was added to the end of the file.

@@ -95,7 +95,7 @@ You can mention him in the relevant thread like this: `@btford`.

> Thanks for submitting this issue!
> Unfortunately, we don't think this functionality belongs in core.
> The good news is that you could easily implement this as a third-party module and publish it on Bower and/or to the npm repository.
> The good news is that you could easily implement this as a third-party module and publish it to the npm registry.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this change? It is about other people publishing to bower. Not us using dependencies from bower. Or are you just trying to move everyone away from bower in every possible way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess I should revert this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to make this change. bower has no immediate advantages over npm, and is getting less important every week.

@@ -13,7 +13,7 @@ var rename = require('gulp-rename');

// We indicate to gulp that tasks are async by returning the stream.
// Gulp can then wait for the stream to close before starting dependent tasks.
// See clean and bower for async tasks, and see assets and doc-gen for dependent tasks below
// See clean for an async task, and see assets and doc-gen for dependent tasks below.
Copy link
Contributor

@petebacondarwin petebacondarwin Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither clean nor bower tasks exists in this file. An example of an async task is doc-gen, but actually I think that one returns a promise...

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor nits but this looks good enough.
I think we should have a README in the vendor folder to explain why we have these vendor files, what versions we have here and how to go about updating them.

@Narretz
Copy link
Contributor

Narretz commented Jan 8, 2018

Rebased with readme as #16394

@Narretz Narretz closed this Jan 8, 2018
@mgol mgol deleted the bower-removal branch January 16, 2018 22:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from bower to yarn aliases Request dependency <=2.68 opens to potential memory exposure vulnerability
4 participants