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

fix: Removed bower #4235

Closed
wants to merge 3 commits into from
Closed

Conversation

Dishebh
Copy link
Member

@Dishebh Dishebh commented Mar 15, 2020

Fixes #3957

Short description of what this resolves:

Migrating away from bower to yarn.

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Mar 15, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/eventyay/open-event-frontend/33m3bdrxh
✅ Preview: https://open-event-frontend-git-fork-dishebh-removebower.eventyay.now.sh

@iamareebjamal
Copy link
Member

There should be no reference of bower_components in the project

Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

RC

@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #4235 into development will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #4235   +/-   ##
============================================
  Coverage        21.88%   21.88%           
============================================
  Files              461      461           
  Lines             4830     4830           
  Branches             5        5           
============================================
  Hits              1057     1057           
  Misses            3771     3771           
  Partials             2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2899022...e605687. Read the comment docs.

@Dishebh
Copy link
Member Author

Dishebh commented Mar 15, 2020

There should be no reference of bower_components in the project

I guess I have changed bower_components to node_modules/@bower_components at all the places. Pls let me know if some place is left out.

@iamareebjamal
Copy link
Member

I can see bower in node_modules/@bower_components. Ctrl+F and search bower in the project. Stop when the search returns 0 results

@iamareebjamal
Copy link
Member

You should always look at unapproved PRs to first check what not to do, so you don't waste your time repeating the same things which another person did already - #3982

@Dishebh
Copy link
Member Author

Dishebh commented Mar 16, 2020

I can see bower in node_modules/@bower_components. Ctrl+F and search bower in the project. Stop when the search returns 0 results

I can see bower in node_modules/@bower_components. Ctrl+F and search bower in the project. Stop when the search returns 0 results

To migrate away from bower and switch to yarn, i.e., to bower-away all the dependencies from bower.json to package.json, isn't it necessary to change any reference to bower_components with node_modules/@bower_components, as mentioned in the link below?

https://bower.io/blog/2017/how-to-migrate-away-from-bower/

@iamareebjamal
Copy link
Member

You need to remove bower components and use npm/yarn dependencies instead

@iamareebjamal
Copy link
Member

See how it was done for raven JS #4091

@Dishebh
Copy link
Member Author

Dishebh commented Mar 17, 2020

You need to remove bower components and use npm/yarn dependencies instead

There's no bower.json file in the project, and removed bower_components from the project directory. But, since yarn is unable to resolve all the bower dependencies, so that's why adding those flattened to package.json. Hence, node_modules/@bower_components is necessary, as when we install package.json with yarn, node_modules/@bower_components will contain all components in exactly the same way they would be installed by Bower.

@Dishebh
Copy link
Member Author

Dishebh commented Mar 17, 2020

If you say, shall I try removing all the code lines containing node_modules/@bower_components as well, and see if the project is running without any additional warnings and errors?

@iamareebjamal
Copy link
Member

It obviously won't work. You have to remove the bower component and add pure npm/yarn component. Tell me which component is not present in npm?

@Dishebh
Copy link
Member Author

Dishebh commented Mar 17, 2020

It obviously won't work. You have to remove the bower component and add pure npm/yarn component. Tell me which component is not present in npm?

But what shall I do about the SemanticUI sources present in environment.js file?

css : 'node_modules/@bower_components/open-event-theme/dist', javascript : 'node_modules/@bower_components/open-event-theme/dist', images : 'node_modules/@bower_components/open-event-theme/dist/themes/default/assets/images', fonts : 'node_modules/@bower_components/open-event-theme/dist/themes/default/assets/fonts'

They are all referring to bower_components in node_modules directory, each of which do not have any file or directory present on that path. Maybe, it is due to running bower-away.

@iamareebjamal
Copy link
Member

Use the directory they are now installed in

@iamareebjamal
Copy link
Member

iamareebjamal commented Mar 17, 2020

If you want to make it easy on yourself. Remove one thing at a time and remove popular dependencies like croppie first

@vercel vercel bot temporarily deployed to Preview March 17, 2020 19:29 Inactive
@Dishebh
Copy link
Member Author

Dishebh commented Mar 17, 2020

How about now?

@lgtm-com
Copy link

lgtm-com bot commented Mar 17, 2020

This pull request introduces 156 alerts when merging 5dd4bcf into 2899022 - view on LGTM.com

new alerts:

  • 119 for Unused variable, import, function or class
  • 16 for Useless assignment to local variable
  • 10 for Unsafe jQuery plugin
  • 4 for Useless conditional
  • 4 for Comparison between inconvertible types
  • 2 for Property access on null or undefined
  • 1 for Superfluous trailing arguments

@iamareebjamal
Copy link
Member

Seriously? You didn't see the 454 files you were about to commit before pushing?

@Dishebh
Copy link
Member Author

Dishebh commented Mar 18, 2020

Seriously? You didn't see the 454 files you were about to commit before pushing?

Honestly, what I just did was npm install all the bower dependencies, remove the @bower_components/node_modules dependencies from package.json, change the path of SemanticUI sources present in environment.js and ember-cli-build.js, and deleted the bower_components folder in the main directory. This, sure, caused a lot of file changes and additions.

@Dishebh
Copy link
Member Author

Dishebh commented Mar 18, 2020

Maybe, I will just try adding and replacing dependencies one-by-one as you said.

dependabot-preview bot and others added 2 commits March 19, 2020 00:33
… to 3.16.1.

- [Release notes](https://github.com/ember-cli/ember-cli/releases)
- [Changelog](https://github.com/ember-cli/ember-cli/blob/v3.16.1/CHANGELOG.md)
- [Commits](ember-cli/ember-cli@v3.16.0...v3.16.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

This reverts commit fff91fb.

Revert commits

This reverts commit 978b6e6.
@Dishebh Dishebh closed this Mar 25, 2020
@Dishebh Dishebh deleted the remove_bower branch March 25, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove bower
2 participants