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

Allow generating --dev projects outside Phoenix source #1224

Closed
wants to merge 3 commits into from
Closed

Allow generating --dev projects outside Phoenix source #1224

wants to merge 3 commits into from

Conversation

jeffkreeftmeijer
Copy link
Contributor

Right now, generating a Phoenix project with the --dev flag throws an error if the generated project is not inside the installer directory in the Phoenix source;

  $ mix phoenix.new /Users/jeff/opensource/phoenix_edge --dev
  Compiled lib/phoenix_new.ex
  Generated phoenix_new app
  ** (Mix) --dev project must be inside Phoenix directory

After poking around for a bit, I wasn’t able to find a reason for this, and I found that projects generated with the --dev flag would work properly anywhere if I just changed the path to Phoenix to an absolute one in the project’s mix.exs file.

So, I removed the code that checks if the project path is inside phoenix/installer and simply returned the absolute path to the Phoenix source when the --dev flag is used, which results in this line in the generated mix.exs file;

  [{:phoenix, path: "/Users/jeff/opensource/phoenix", override: true},

That eliminates the "--dev project must be inside Phoenix directory”-error and cleans up installer/lib/phoenix_new.ex quite a bit. What do you think? Did I miss anything?

@josevalim
Copy link
Member

Hey @jeffkreeftmeijer, the issue I ran with this was related to brunch. I believe brunch can't find your assets if you use absolute paths. Try scaffolding a resource in an app with --dev and see if the delete button works and see if Phoenix.Socket is there.

@josevalim
Copy link
Member

And thanks for the PR!

@jeffkreeftmeijer
Copy link
Contributor Author

Hey @josevalim, thanks for the pointers and your quick reply. Brunch worked fine for me, so I did some testing and found that the assets were generated properly on Brunch 1.8.5, but not on earlier versions.

So, depending on "^1.8.5" instead of "^1.8.1" would solve this issue. Do you want me to change that in package.json?

@josevalim
Copy link
Member

Yes, please. I am also worried about this:

https://github.com/jeffkreeftmeijer/phoenix/blob/dev-projects-outside-phoenix-directory/installer/lib/phoenix_new.ex#L466

This means the brunch file has a path to "../Users/jeff/..". It would be really weird if that is working. If you can check that too, it would be great, if not, I can check it before merging. Thank you!

@jeffkreeftmeijer
Copy link
Contributor Author

After some more testing, I found Phoenix.Socket was indeed missing, caused by using the absolute path to phoenix.js in the Phoenix source in web/static/js/app.js;

  import {Socket} from "/Users/jeff/opensource/phoenix/web/static/js/phoenix"

It works with a relative path, so I think our best bet is to reinstate phoenix_static_path and have that return a relative path from the project directory to the phoenix source directory. What do you think?

Also, I tried finding something like Ruby's Pathname#relative_path_from in Elixir, but couldn't find it. Does a function like that exist, before I reinvent the wheel?

@josevalim
Copy link
Member

We have Path.relative_from but it is not guaranteed to give a relative path if they are two distinct path structures. In other words, if you have it /foo/bar and /bat/bat, it won't make it ../../foo/bar because they are disjoint.

At this point, I am wondering if it is worth to continue chasing this. The --dev flag is a development only thing, so we can try things out, and requiring the same filepath seems like an OK restriction.

@jeffkreeftmeijer
Copy link
Contributor Author

Yes, I found Path.relative_from, but wondered if I was missing anything.

This seemed like a simple thing to do as my first patch, but I agree this has gotten out of hand a little, so I'll close this. Thanks a lot for your help!

@wsmoak
Copy link
Contributor

wsmoak commented Sep 20, 2015

Thanks for exploring whether this could be changed! #1227 proposes adding more info to the README so the next person along doesn't have to re-discover it. :)

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

Successfully merging this pull request may close these issues.

3 participants