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 support and dist/* files #3033

Closed
schmunk42 opened this issue Jul 26, 2016 · 26 comments
Closed

Bower support and dist/* files #3033

schmunk42 opened this issue Jul 26, 2016 · 26 comments
Assignees

Comments

@schmunk42
Copy link

Would it be possible to re-add bower.json?

Removed here: c631874

It breaks several things, like:
2amigos/yii2-chartjs-widget#18
fxpio/composer-asset-plugin#242
bedezign/yii2-audit#159

@panzarino
Copy link
Contributor

You can still install with bower through the bower-npm-resolver as explained in the installation portion of the docs.

@etimberg
Copy link
Member

Further, with the build system changes this cannot be installed via bower because the release source code zip doesn't contain the built files

@schmunk42
Copy link
Author

We're installing the plugin via https://github.com/fxpio/composer-asset-plugin - so I don't think npm-bower-resolver would work here. Probably the best option for the widget is to switch to npm.

Just to be curios, what was the reason behind dropping bower-support, like maintaining two package managers or did bower just not work fine?

@fabianTMC
Copy link

I am curious as well as to why bower support was dropped.

@madmoizo
Copy link

Indeed, dist folder was useful for dependencies management, angular wrappers or whatever

@panzarino
Copy link
Contributor

You can get the dist folder through building with gulp build.

@koxon
Copy link

koxon commented Aug 2, 2016

Could you gulp build for us?

This adds another step to the deployment process. And I'm not planning to add gulp just for one dependency.

@miguelmich
Copy link

There is a way to include the dist folder only on releases even if this folder is ignored by default, this way it will be available when checkout releases tags, popular repos like jQuery or full calendar manage distribution on releases like this.

I guess you are currently manually adding the compiled files on each release. However you could add this behavior automatically in your release task in gulp, this way everybody will be happy and no dist folder will be in source control, only on releases :)

What do you think?

@etimberg
Copy link
Member

etimberg commented Aug 3, 2016

@miguelmich the release already happens automatically. @simonbrunel set it up so that it adds the built files once the build finishes. As far as I know, there is no way to have those files inside the source zip (which is what bower uses) without checking those files in.

@simonbrunel
Copy link
Member

simonbrunel commented Aug 3, 2016

@schmunk42 @fabianTMC: main reasons why we removed dist files (and thus bower support):

  • not obvious which version was compiled (in master/branches)
  • confusing because these files were not always up to date with latest sources
  • pollutes commit history and PR since many people were committing their own built files in PR
  • makes more complicated manipulating git history for developers, such as rebasing

@koxon: we already build them for you, no need to deal with gulp or even npm. You can get it from the Git release page or the CDN (or the npm package if you use npm). We provide many formats, see #2555 for details.

@miguelmich: the only manual process is the Git tag creation, everything else is automated: build, git release (attached files), NPM release and deployment to the CDN (described in #2555). But I see what you mean with jQuery, they have the Git tag automatically created and in the same time they push dist files only for the tag. So finally, dist files are never on master/branches, neither PRs. Could be something interesting to investigate :)

I don't know bower enough, but I'm sure there are different ways to grab libraries from URL (Git release downloads or CDN) without relying on Git repository, right?

@miguelmich
Copy link

Hello @etimberg @simonbrunel,

Ok, now I understand better your workflow, however one of the main concerns about using direct urls is that we can't manage version tags.

It is also possible to include the dist folder only in the releases tags and master branch will be clean always for PRs like mentioned here: (fullcalendar/fullcalendar#2989 (comment))

This could be an addition to the current build process instead of some kind refactor, If you want I can make a research more deeply about this and give you a better idea of how it could work.

Thanks

@miguelmich
Copy link

Hello guys,

I want to share what I found so far; The behavior mentioned above is possible to achieve following these steps in the release task:

  • Run build task
  • Checkout a detached state
  • Add dist folder files to commit
  • Commit these files
  • Create a tag
  • Push tag to origin
  • Reset the detached state
  • Checkout HEAD

This process is explained in more detail here, and these two repos are using the same releasing workflow as you can see in their release tasks:
https://github.com/fullcalendar/fullcalendar/blob/master/build/build-release.sh
https://github.com/melonjs/melonJS/blob/master/tasks/release.js

Both are using a symbolic reference pointing to HEAD (git symbolic-ref HEAD) in order to return to the previous branch prior to the detached state.

@simonbrunel
Copy link
Member

Thanks @miguelmich, will look it more closely, however our current build/release process is triggered from the git tag creation, so this solution doesn't seem to work out of the box and would require other major changes in our workflow.

The first link you posted is from 2014, but since there are alternatives, such as bower-npm-resolver. So I wondering why this solution is not suitable?

@miguelmich
Copy link

Ok I understand your point of view, but personally I don't like to install additional plugins to handle a default behavior in bower using a "workaround" just for one dependency.

If you guys decided to not support bower you should have your reasons and I respect that :)

@trading-peter
Copy link

I too respect your decision to drop bower support. What's naughty though is that you guys do that without respecting semver. Many bower based projects use the semver range ^2.x.x, expecting braking changes to occur in major version changes only. Now many bower based projects install version 2.2.x with the braking change of not having build files.

@simonbrunel simonbrunel changed the title Re-add bower.json Bower support and dist/* files Aug 4, 2016
@simonbrunel
Copy link
Member

@miguelmich: nothing decided yet, we need to be sure that there is no other way than dealing with dist files in the repository tags to support Bower. It will potentially impact the whole Chart.js release workflow, so it needs to worth it (meaning that alternatives solutions are really not acceptable). I can understand why you don't want to use bower-npm-resolver and I'm not sure why Bower doesn't support npm packages natively since it requires npm itself. IMHO, bower-npm-resolver should be built in Bower.

... main concerns about using direct urls is that we can't manage version tags

What do you think about using npmcdn.com for version management, would it work for you? For example, you could use:

  • bower install https://npmcdn.com/chart.js@2.x.x/dist/Chart.js
  • bower install https://npmcdn.com/chart.js@^2.2.0/dist/Chart.js
  • ...

@pkaske you absolutely right :\ I guess the confusion came from that the lib itself is still compatible with 2.x.x, but only Bower integration needs adaptation.

Note: reopening this issue to keep track of the discussion about Bower support.

@simonbrunel simonbrunel reopened this Aug 4, 2016
@miguelmich
Copy link

Yes, I think I can use the cdn url since it's only one file

@CyborgMaster
Copy link

Another small plug for native bower support. We include bower packages inside of rails using rails-assets.org, which re-packages bower packages as Ruby gems. This only works if it's a native bower package and doesn't use any extra plugins like npm-resolver or custom urls. I'm not suggesting that you do any extra work for just that reason, because this workflow is pretty niche, but this is one more small reason why native bower support with a dist folder would be nice for some people

@maxim-danilov
Copy link

maxim-danilov commented Aug 10, 2016

I'm use bower install npm:chart.js --save, but wiredep (gulp plugin) in my project incorrect inject chartjs:
<script src="/bower_components/chart.js/src/chart.js"></script>
and i get error: chart.js:4 Uncaught ReferenceError: require is not defined

P.S. I have dist folder

@panzarino
Copy link
Contributor

@700ghz You are using the wrong version of Chart.js. The only versions that you should be using for production are in the dist folder. You should change your src to be /bower_components/chart.js/dist/Chart.min.js. (use Chart.bundle.min.js if you want to use time scale)

@maxim-danilov
Copy link

maxim-danilov commented Aug 22, 2016

In file "bower.json" we have:

{
  "name": "chart.js",
  "description": "Simple HTML5 charts using the canvas element.",
  "main": "src/chart.js",
  "license": "MIT",
  "homepage": "http://www.chartjs.org",
  "repository": {
    "type": "git",
    "url": "https://github.com/chartjs/Chart.js.git"
  },
  "dependencies": {},
  "devDependencies": {}
}

In line:
"main": "src/chart.js"
should be:
"main": "dist/Chart.js"

Now gulp wiredep work correctly.

@panzarino
Copy link
Contributor

We are implementing a new system very soon that should allow full bower implementation, just as it used to be before the recent update.

@CyborgMaster
Copy link

@zachpanz88 Awesome! I'm excited to be able to upgrade to the latest version. Thanks!

cpitzak added a commit to cpitzak/weatherWeb that referenced this issue Aug 26, 2016
… fix jshint error.

- Changed angular-chart.js to come npm rather than bower because angular-chart.js from bower grabs the latest version of the Chart.js and the Chart.js team decided to no longer provide a distribution of their library dependency. Chart.js latest update no longer provides a dist/ folder of files. Which obviously breaks support for bower. They will be readd support in the next version, until then using the dependency from npm because the support is still there. chartjs/Chart.js#3033
- Adding grunt to copy over dependency files.
simonbrunel added a commit to simonbrunel/Chart.js that referenced this issue Aug 28, 2016
Add a new Travis deploy task to push dist/*.js and bower.json files into tag sources:
- requires Travis GITHUB_AUTH_TOKEN and GITHUB_AUTH_EMAIL environment variables
- skipped if not built from the "release" branch
- release.sh must be executable (see comment)
- reads tag version from package.json
- fails if tag already exists
simonbrunel added a commit to simonbrunel/Chart.js that referenced this issue Aug 28, 2016
Add a new Travis deploy task to push dist/*.js and bower.json files into tag sources:
- requires Travis GITHUB_AUTH_TOKEN and GITHUB_AUTH_EMAIL environment variables
- skipped if not built from the "release" branch
- release.sh must be executable (see comment)
- reads tag version from package.json
- fails if tag already exists
simonbrunel added a commit to simonbrunel/Chart.js that referenced this issue Aug 28, 2016
Add a new Travis deploy task to push dist/*.js and bower.json files into tag sources:
- requires Travis GITHUB_AUTH_TOKEN and GITHUB_AUTH_EMAIL environment variables
- skipped if not built from the "release" branch
- release.sh must be executable (see comment)
- reads tag version from package.json
- fails if tag already exists
@simonbrunel simonbrunel self-assigned this Sep 17, 2016
@simonbrunel
Copy link
Member

simonbrunel commented Sep 21, 2016

Fixed in version 2.3.0, no need to use bower-npm-resolver anymore, native Bower support is back.

@simonbrunel
Copy link
Member

Thank you @miguelmich for pointing us on that process to push dist/* files along release tags :) (our implementation is quite similar to the fullcalendar one and is automated by Travis).

@miguelmich
Copy link

@simonbrunel I'm glad to hear that, you have done a great work guys.

exwm pushed a commit to exwm/Chart.js that referenced this issue Apr 30, 2021
Add a new Travis deploy task to push dist/*.js and bower.json files into tag sources:
- requires Travis GITHUB_AUTH_TOKEN and GITHUB_AUTH_EMAIL environment variables
- skipped if not built from the "release" branch
- release.sh must be executable (see comment)
- reads tag version from package.json
- fails if tag already exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests