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

interface npm scripts #4291

Closed
wants to merge 5 commits into from
Closed

interface npm scripts #4291

wants to merge 5 commits into from

Conversation

c4710n
Copy link
Contributor

@c4710n c4710n commented Apr 12, 2021

scripts section of package.json is a good place to provide commands which are used by Phoenix.

If we abstract the long commands into short npm scripts, we will:

  • have a unified interface to call node.js related commands.
  • avoid the long commands(such as node node_modules/webpack/bin/webpack.js --mode development) spreading in the code

And, there are two more advantage:

  • The developers who want to implement his own assets dir will have a better view on what they should implement - just provides necessary NPM scripts, no need to touch the Phoenix code.
  • When Phoenix Team decide to upgrade to Webpack 5, just update the npm scripts, there's nothing need to do with the Elixir code.

@josevalim
Copy link
Member

This would be our preferred approach but every time we made this change people reported issues, especially on OSes like Windows. So if someone can try this out on a Windows machine (under WSL, mingw, cygwin, etc), then we can merge it, otherwise it is probably best to leave it as is. :(

@c4710n
Copy link
Contributor Author

c4710n commented Apr 19, 2021

I will try this out on a Windows machine like you said.

@c4710n
Copy link
Contributor Author

c4710n commented Apr 19, 2021

Running modified version phx.new

$ git clone -b adjust-assets https://github.com/c4710n/phoenix.git
$ cd phoenix/installer
$ mix deps.get
$ mix compile
$ mix phx.new /tmp/demo

WSL

  • Sub System: Ubuntu 20.04.2 LTS
  • Elixir: 1.11.2 (Following the installation at here)
  • Node.js: 10.19.0 (installed by sudo apt-get install nodejs)
  • npm: 6.14.4 (installed by sudo apt-get install npm)

WSL works like a generic Linux box, no issue found.

web installer

  • Git: 2.3.1 (installed by installer provided by https://git-scm.com)
  • Elixir: 1.11.2 (installed by installer by Elixir team)
  • Node.js: 14.16.1 (installed by installer provided by Node.js team)
  • npm: 6.14.2 (included in above installer)

After the installation, elixir mix npm can be found in the PATH environment variable.

But, error occured, just like #1218.

I think that's what you said above.

After serveral hours, I think I can't fix this problem.

mingw / cygwin

Ignore them for now.

The above problem has not been solved, and there is no point in solving this problem. ;(

Conclusion

Although this works on Linux, macOS, WSL , but not works on bare Windows.

It seems there's no one perfect solution. I will give up this PR.

Finally, thanks for letting me know this issue. ;)

@c4710n c4710n closed this Apr 19, 2021
@josevalim
Copy link
Member

Thanks for checking, it is very appreciated! <3

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.

None yet

2 participants