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

Remove browser builds from source #60

Closed
imor opened this issue Jun 4, 2014 · 15 comments
Closed

Remove browser builds from source #60

imor opened this issue Jun 4, 2014 · 15 comments

Comments

@imor
Copy link
Collaborator

imor commented Jun 4, 2014

Files lib/pathfinding-browser.js and lib/pathfinding-browser.min.js should be removed. These are release artefacts which should be produced only when making a release. For people to have ready access to these files they should be uploaded on the repo's release page.

Benefits of doing this are -

@qiao What do you say?

@qiao
Copy link
Owner

qiao commented Jun 4, 2014

Hi @imor, I fully agree with you on setting up releases for the distribution files. And thank you for all your hard work on maintaining this project 👍

UPDATE: I've created a release and modified the README to let users download from the releases page.

@imor
Copy link
Collaborator Author

imor commented Jun 5, 2014

Closing the issue then :)

@imor imor closed this as completed Jun 5, 2014
@vasa-chi
Copy link

vasa-chi commented Jun 6, 2014

After this change there is no way to import this project with bower. That is really problematic.

@imor
Copy link
Collaborator Author

imor commented Jun 6, 2014

@vasa-chi I'm not sure what you mean exactly because I'm not too familiar with bower but if you are trying to install pathfindingjs doesn't the following command work?
bower install https://github.com/qiao/PathFinding.js/releases/download/0.4.11/pathfinding-browser.min.js

@vasa-chi
Copy link

vasa-chi commented Jun 9, 2014

Surprisingly, it works... But the normal workflow would be to install through bower install PathFinding.js. That should clone the latest release from the repo. Since there are no files for browser in the repo, there is no way to use PathFinding.js, unless you compile it manually, which defeats the whole idea of dependency management.

@imor
Copy link
Collaborator Author

imor commented Jun 9, 2014

I don't like keeping release artifacts around in the source code.

When I do bower install PathFinding.js why does it pull the whole source code from the git repo? What a waste. In contrast npm keeps the release artifacts in a separate database from the source repo. A release manager explicitly pushes to this database when making a release and files from only that database are fetched when doing npm install <package>. Can't I somehow tell bower to pull a certain file though http when doing bower install PathFinding.js? If this is not possible then another option is to have a release only repo somewhere which contains only the minified file and nothing else. Ideas anyone?

Anyway I'm reopening this issue until a solution to this problem is found.

@imor imor reopened this Jun 9, 2014
@chrisl8888
Copy link

I'm having this issue as well. when i perform a bower install it doesn't produce a packaged file for me to consume.

May i suggest a Seperate folder structure?

Also, in the time being i would suggest adding documentation on the command you just provided:

bower install https://github.com/qiao/PathFinding.js/releases/download/0.4.11/pathfinding-browser.min.js

My largest issue with doing the install (like above) is that it provides a folder with the wrong name of the repository:

selection_002

Additionally, as you move to different minor and major versions it'll not provide the latest file.

One last suggestion. Change the project from pathfinding.js -> pathfindingjs or pathfinding. This will make globbing easier: [bower_components/*.js].

@cirocosta
Copy link

This is kind of a 'problem' (see this discussion around a postinstall on bower - comparing to npm, where we can specify a script to run after the install) of bower. I say kind of because bower really does not expect to run a build for something as i understand.

IMO the unique way of dealing with it with bower is to actually have the lib dir with the files built.

See jquery - Build: drop bower; use npm for front-end deps


Just created a PR related: #66

@imor
Copy link
Collaborator Author

imor commented Nov 22, 2014

To support bower:

  1. A separate repo will be created (maybe call it pathfinding-bower or something) which contains only the browser builds.
  2. Will be re-registered with the name pathfinding on bower.io. This will prevent globbing issues as pointed out by @chrisjlee and will be consistent between npm and bower.

@imor
Copy link
Collaborator Author

imor commented Nov 22, 2014

It is now possible to install using bower install pathfinding.
Ping @qiao @vasa-chi @chrisjlee @cirocosta. Could you all please give feedback if this works for you before I close this issue.

@RichardJohnn
Copy link

I had to add this to my bower.json :

"overrides": {
    "pathfinding": {
      "main": "pathfinding-browser.js"
    }
  }

but even so, there is no PF defined as in new PF.Grid(5, 3).

As a comparison, I tried easystarjs and also had to add an override to their .js file in bin/ and then new EasyStar.js() just worked

@imor
Copy link
Collaborator Author

imor commented Jan 4, 2015

@RichardJohnn Could you please post your folder structure i.e. where is pathfinding installed and how you import it in your webpage?

@imor
Copy link
Collaborator Author

imor commented Jan 5, 2015

Hi @RichardJohnn , thanks for reporting. Build 0.4.13 was broken as it it did not define the PF variable. I have released 0.4.14. Please try installing again and it should work.

Thanks,
imor

@RichardJohnn
Copy link

yes, that did the trick! also, it looks like your bower.json is good to go too, the 'overrides' section in my project's bower.json is not needed anymore

I'd say this issue is resolved.

@imor
Copy link
Collaborator Author

imor commented Jan 6, 2015

Thanks @RichardJohnn for your feedback 👍 . Closing the issue.

@imor imor closed this as completed Jan 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants